620777 - Ifdef comment cleanup script

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Tools, enhancement)

Description

[Alex Macdonald]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_5; en-us) AppleWebKit/533.19.4 (KHTML, like Gecko) Version/5.0.3 Safari/533.19.4
Build Identifier: 

This simple script updates the comments on "#endif" and "#else" directives.

I'm not sure what tamarin's current style guidelines say about the comments used on ifdefs so I've encoded the following rules into it for now, the script could easily be changed to accommodate other styles:

- if the "if" and "else/end" appear within 4 lines of each other no comment is needed (threshold is adjustable)
- comments are all single line comments "//"
- the comment always represents the condition use to enter the preceding if/ifndef/elif
- the comment used after an ifndef will be formatted as follows "ifndef FOO --> // !(FOO)"

I've attached two patches one patch was created by the script in updateonly mode where it only amends existing but incorrect comments, the other is run in the normal mode which also adds missing comments and remove unneeded ones (per the threshold)

Reproducible: Always

Comment 1

[Alex Macdonald]

Attachment: ifdef cleanup script

Comment 2

[Alex Macdonald]

Attachment: full patch

Comment 3

[Alex Macdonald]

Attachment: partial patch - only amends existing ifdef comments

Comment 4

[Alex Macdonald]

Attachment: ifdef cleanup script

fix a bug with whitespace handling

Comment 5

[Alex Macdonald]

Attachment: partial patch - only amends existing ifdef comments

Comment 6

[Edwin Smith]

Comment on attachment 499151 [details] [diff] [review]
ifdef cleanup script

This is really cool.  Steven - what do you think?  landable?

Comment 7

[Alex Macdonald]

(In reply to comment #6)
> Comment on attachment 499151 [details] [diff] [review]
> ifdef cleanup script
> 
> This is really cool.  Steven - what do you think?  landable?

Before we get to that stage we should probably decide exactly what the best rules for comments on the else/endif directives are, should the just be the same string as the if/else/ifndef or should they be suitably negated so they indicate the condition that would get you into the preceding branch etc

Also, I kept working on this script and it evolved into something much more which I'm now keeping here: http://code.google.com/p/ifdef-refactor/

Essentially I've written a pre-processor capable of rewriting ifdef expressions and removing dead branches based on whether ifdefs are known to be always on or always off.. perhaps not as useful to the vm team, but I intend to use that to refactor some of the player code.

Comment 8

[Edwin Smith]

Cool.

Replacing a dead #if branch with an #error might also be a good idea.

Comment 9

[Steven Johnson]

Comment on attachment 499151 [details] [diff] [review]
ifdef cleanup script

I like it. One nit: I find that

  #ifndef FOO
  #endif // !(FOO)

might be misleading, as my first thought was that this comment was for

  #if !FOO
  #endif // !(FOO)

In this case perhaps just be explicit:

  #ifndef FOO
  #endif // ifndef FOO

?

Comment 10

[Alex Macdonald]

(In reply to comment #9)
> Comment on attachment 499151 [details] [diff] [review]
> ifdef cleanup script
> 
> I like it. One nit: I find that
> 
>   #ifndef FOO
>   #endif // !(FOO)
> 
> might be misleading, as my first thought was that this comment was for

perhaps, but these are equivalent right? I'm happy with either.

what about the following case:

#if something
#elif somethingelse
#endif // what goes here? somethingelse?

and the case where it's an else?

#if something
#else // what goes here? something? or !(something)?
#endif // what goes here? something? or !(something)?

Comment 11

[Steven Johnson]

(In reply to comment #10)
> perhaps, but these are equivalent right? I'm happy with either.

Er, no:

#define FOO 0
#ifndef FOO // -> false
#if !FOO // -> true

Comment 12

[Alex Macdonald]

(In reply to comment #11)
> (In reply to comment #10)
> > perhaps, but these are equivalent right? I'm happy with either.
> 
> Er, no:
> 
> #define FOO 0
> #ifndef FOO // -> false
> #if !FOO // -> true

oops, misread your if as an ifdef. so the comment for an ifndef should either be "!defined(FOO)" or "ifndef FOO"

Comment 13

[Steven Johnson]

(In reply to comment #12)
> oops, misread your if as an ifdef. so the comment for an ifndef should either
> be "!defined(FOO)" or "ifndef FOO"

either is fine with me.

Comment 14

[Alex Macdonald]

Attachment: rewrite ifdefs to follow a common style

Comment 15

[Alex Macdonald]

Attachment: rewrite ifdef comments to follow a common style

Comment 16

[Alex Macdonald]

(In reply to comment #13)
> (In reply to comment #12)
> > oops, misread your if as an ifdef. so the comment for an ifndef should either
> > be "!defined(FOO)" or "ifndef FOO"
> 
> either is fine with me.

Ok, I've changed the script so that ifdef/ifndef will be mention in the comment to differentiate them from pure expressions.

I've also regenerated two new patches with the tool. The first patch normalizes conditional expressions so that:

- a conditional that checks whether one macro is defined always uses an "ifdef" e.g. "#if defined(FOO)" should be written as "#ifdef FOO"
- a conditional that checks whether one macro is not defined always uses an "ifndef" e.g. "#if !defined(FOO)" should be written as "#ifndef FOO"
-parens around identical associative operators are not needed e.g. "A || (B || C)" should be written as "A || B || C"
-parens around different operators must be used e.g. "A && B || C" should be written as "(A && B) || C"
-there shouldn't be any parens around the entire expression e.g. "#if (A > 3)" should be written as "#if A > 3"
- A "defined" expression should be of the form "defined(FOO)" e.g. parens must be present without any unnecessary spaces.

The second patch updates the comments on "else/endif" directives so that:

-A comment on an else/endif will only be generated if it is further than four lines from the preceding directive
- The comment on an else/endif directive shows the conditional used on the preceding if/ifdef/ifndef/elif directive
- The comment generated for a preceding "if/elif" directive is simply the contents of the conditional
- The comment generated for a preceding "ifdef/ifndef" directive is prefixed with the token name e.g. "#endif // ifdef foo"
- All comments are C-style single line comments

Comment 17

[Edwin Smith]

Comment on attachment 506306 [details] [diff] [review]
rewrite ifdefs to follow a common style

Morally, this is great.

Pragmatically, since the tool is evolving, and not in tamarin, what is a good way to ensure we can run the tool again on modified code, and get similar results?  a README is one way (where?).  Another more expensive idea is to leave a modeline at the top of modified files, containing the settings the tool should use.  (and make the tool understand the modeline).

The patches should be landed in parts, most likely.  There's a lot of churn in MMgc, I expect we'll want to hold that off until a convenient point, where it will be easier to re-run the tool than to regenerate the patch.

I'd really like it if these changes were bringing code into conformance with a written style guide, but I won't look a gift horse in the mouth.  hopefully when said guide comes into existence, its alraedy consistent with this patch.

I don't think its worth the time to break up and re-review the patches, I think whoever lands these should just rerun the tool in the required directories as separate checkins.

suggested breakup, in no particular order:

* platform, VMPI, utils, vmbase
* shell, extensions, eval
* core
* MMgc
* nanojit, vprof  (will have to submit to nanojit-central separately)

adding NJN for feedback on the nanojit part.

Comment 18

[Nicholas Nethercote [inactive]]

Comment on attachment 506306 [details] [diff] [review]
rewrite ifdefs to follow a common style

Does this solve any real problems?  I'm not opposed to it, doesn't seem like it's really worth much effort, but I won't stop you.

Comment 19

[Edwin Smith]

No more or less than any other code cleanup activity - whitespace, poor indentation, etc.  Clean code has some intangible value, but could cause merge churn.  The patch is output from a tool, low work, but speak up if it would be more convenient to delay until after FF4 releases, for example.

Comment 20

[Lars T Hansen]

Comment on attachment 506306 [details] [diff] [review]
rewrite ifdefs to follow a common style

> - A "defined" expression should be of the form "defined(FOO)" e.g. parens
> must be present without any unnecessary spaces.

I don't care for this one bit - the parens are entirely redundant - but it's hardly worth bickering over.

Comment 21

[Dan Smith]

Project appears abandoned.  Status?

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.