Open Bug 1523793 Opened 6 years ago Updated 2 years ago

Introduce macro to annotate definitions of previously-declared static/virtual symbols

Categories

(Developer Infrastructure :: Source Code Analysis, enhancement, P3)

enhancement

Tracking

(Not tracked)

People

(Reporter: mozbugz, Unassigned)

References

Details

Spawned from discussion:
https://groups.google.com/d/msg/mozilla.dev.platform/6_reFXkmZIA/nFCjZoRGDAAJ

The initial discussion was around the common pattern where static or virtual methods have a comment reflecting this attribute before the definition, but because of the new google-style formatting, these comment now take more valuable horizontal space and push the important method name further to the right, e.g.:
/* static */ void Foo::Bar() {

This made me think that maybe instead of fragile comments (which could confusingly blend with surrounding comments, or more dangerously become false if the declaration was changed), we could have blank macros that are easier to spot, and could be kept correct thanks to a clang checker.
E.g.:
DECLARED_STATIC void Foo::Bar() {

(Note that this doesn't help with the issue of horizontal spacing!)

This could be introduced in phases:

  1. Add DECLARED_STATIC and DECLARED_VIRTUAL (and maybe DECLARED_OVERRIDE, DECLARED_FINAL?) blank macros.
  2. Start using these macros.
  3. Add clang checker that verifies that DECLARED_... are correct.
  4. Mass-change comments into these macros where relevant.
  5. (Optional) Make clang checker stronger, to enforce using these macros (i.e., the definition of a statically-declared symbol must have a DECLARED_STATIC), similar to how MOZ_IMPLICIT is required for non-explicit constructors.

If agreed, this bug could serve as meta-bug for these phases.

I'm told that clang-format doesn't actually touch these macros if already on separate lines. So if we have:

DECLARED_STATIC
void Foo:: Bar() {

it will not be reformatted to put everything on one line.

So this should help with the issue of horizontal spacing (from bug 1523969, if that doesn't get fixed first).

However note that if they are already on the same line, they will also be left untouched (DECLARED_STATIC void Foo::Bar() {).
But maybe the static checker could check for that too, and enforce having separate lines? (Should it?)

The priority flag is not set for this bug.
:sylvestre, could you have a look please?

Flags: needinfo?(sledru)
Flags: needinfo?(sledru)
Priority: -- → P3
Product: Firefox Build System → Developer Infrastructure
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.