Clarify our #if{n}def by commenting what they are at the #elif/#else/#endif

Ok that's an easy one!

The idea is to improve our comments in the .h/.c files where we have a #ifdef BLAH and the corresponding #endif is at least a non negligible amount of lines after. Of course that goes without saying to also comment the #ifndef. Pattern would look like this:

#ifdef BLAH
[some non negligible amount of lines]
#elif /* BLAH */
[some other amount of lines]
#endif /* BLAH */

According to nickm, the current situation is:

Our headers have 27 #endifs with a comment, and 435 without.

Good example is or.h:

#ifndef TOR_OR_H
[5000+ lines in between]
#endif /* TOR_OR_H */

This file src/or/shared_random.h is a good example of what it could look like.

changed milestone to %Tor: 0.3.2.x-final

added actualpoints::.1 component::core tor/tor easy lorax milestone::Tor: 0.3.2.x-final owner::cjb priority::very low resolution::fixed review-group-23 reviewer::dgoulet severity::trivial status::closed type::enhancement labels

I wonder if there's a tool that'd do this programmatically! (At least for a first pass.)

Here's one I just found, it's a vim script:

https://sites.google.com/site/abudden/contents/Vim-Scripts/smart-brace--preprocessor-commenting/smartcommenter.vba?attredirects=0&d=1

I'd guess it's straightforward to make it count the number of lines between the if/else/elif and their endif, and suppress the output if that's fewer than N lines.

I'd guess it's straightforward to make it count the number of lines between the if/else/elif and their endif, and suppress the output if that's fewer than N lines.

I've done this now. I'll attach my changes to smartcommenter.vba as well as the patch against the Tor source. Some questions:

1. I set the minimum number of lines between #if..#endif before introducing an comment to be 20. Should it be larger? My thinking was that 20 lines is about a screenful at default terminal size.

2. In this stanza:

#ifdef FOO
...
#else /* FOO */
...
#endif /* FOO */

Should the #else comment be FOO, or !(FOO)?

3. In this stanza:

#ifndef FOO
...
#else /* FOO */
...
#endif /* FOO */

Should the #endif comment be FOO or !(FOO)?

4. The vim plugin is capable of annotating long braced/indented sections too. Is that attractive?

Trac:
Owner: N/A to cjb
Status: new to accepted

Trac:
smartcommenter.vba

vim plugin for #endif annotation

Trac:
annotate-endifs.patch

endif annotation patch v1

Trac:
Status: accepted to needs_review

Trac:
Milestone: Tor: 0.2.??? to Tor: 0.2.9.x-final

This seems worth doing. I think we should use the script to regenerate this closer to our 0.2.9 release, though, so we can minimize conflicts.

Trac:
Status: needs_review to merge_ready

Deferring big/risky-feature things (even the ones I really love!) to 0.3.0. Please argue if I'm wrong.

Trac:
Milestone: Tor: 0.2.9.x-final to Tor: 0.3.0.x-final
Keywords: N/A deleted, nickm-deferred-20161005 added

Trac:
Keywords: N/A deleted, review-group-11 added

Trac:
Keywords: review-group-11 deleted, N/A added

Might be a good time to apply this one. I volunteer to fix the merge conflicts that this will cause down the road.

I tried running this with:

for fn in src/common/*.[ch] src/or/*.[ch] src/test/*.[ch] src/tools/*.[ch] ; do
   vim -c ':SmartPreProcCommenter' -c ':wq' $fn; 
done

I'm getting some pretty weird results though -- for instance, in compat.c, it makes this:

#if !defined(_WIN32)
/** Defined iff we need to add locks whena defining fake versions of reentrant
 * versions of time-related functions. */
#define TIME_FNS_NEED_LOCKS
#endif /* HAVE_GMTIME_R, defined(TIME_FNS_NEED_LOCKS), defined(TIME_FNS_NEED_LOCKS) */

That can't be right, can it?

Trac:
Status: merge_ready to needs_revision

Yeah, I saw that too and fixed those up by hand last time around (happy to do so again). Not sure what's up with that either.

Hm, actually I can't reproduce your result. Might you be using the plugin from the sites.google.com link I posted, rather than my modified one that's attached to this ticket?

The reason I suspect that you're using the unmodified plugin is that it annotated a three-line block for you, but my modification is supposed to limit annotations to >20 line blocks, which it seems to be doing here -- the block you quoted isn't annotated at all when I run the plugin myself.

(I suppose it's also possible that we're seeing different vim behaviors somehow.)

Trac:
Keywords: N/A deleted, review-group-15 added

Trac:
Reviewer: N/A to nickm

Trac:
Status: needs_revision to needs_review

Yeah I can see in the patch:

#endif /* defined(HAVE_MLOCKALL) && HAVE_DECL_MLOCKALL && defined(RLIMIT_MEMLOCK) */

Ideally, we don't want those "defined()" in there... Apart from that, the changes looks good so I'm deferring this to 031 but if by 030 stable we have a new patch version fixing the above, let's consider it! This is just comment change.

Trac:
Milestone: Tor: 0.3.0.x-final to Tor: 0.3.1.x-final
Status: needs_review to needs_revision

Trac:
Keywords: nickm-deferred-20161005 deleted, N/A added

Trac:
Keywords: review-group-15 deleted, N/A added

Trac:
Milestone: Tor: 0.3.1.x-final to Tor: 0.3.2.x-final

Hm. So, let's try again. I don't think we can require a specific vim plugin if we're going to have this be a regular part of our toolchain. So how about we try it in Python? I've put a script in an annotate_ifdefs branch in my public repository that I think does just about everything we want. I've put a sample of its output in annotate_ifdefs_sample_output, but we should re-run it immediately on merge if we decide to take it.

Trac:
Status: needs_revision to needs_review
Actualpoints: N/A to .1
Reviewer: nickm to N/A

Put 0.3.2 needs_review and merge_ready tickets into review-group-23.

Trac:
Keywords: N/A deleted, review-group-23 added

May I request a change, I know it's super nitpick but it's a personal preferences that we use /* ... */ instead of the // ?

Another thing, seems that top level ifdef are working with the script but not the one inside. If I take statefile.h for instance, #ifdef STATEFILE_PRIVATE its #endif doesn't the annotation. Bug or on purpose? Which is weird because I see other file correctly annotated...

Trac:
Status: needs_review to needs_revision

We can do /* ... */, and I tried that at first, but it will make a lot more lines that are longer than 80 columns and need to get truncated. Still want it?

I think the issue you're reporting with statefile.h is not about outer vs inner, but about the rule that we don't annotate endifs that are within 4 lines of their ifs. (See LINE_OBVIOUSNESS_LIMIT in the script.)

Replying to nickm:

We can do /* ... */, and I tried that at first, but it will make a lot more lines that are longer than 80 columns and need to get truncated. Still want it?

I made a test, I see 10 long line with // and 17 with /* */. I even volunteer to fix those after merge :P.

I think the issue you're reporting with statefile.h is not about outer vs inner, but about the rule that we don't annotate endifs that are within 4 lines of their ifs. (See LINE_OBVIOUSNESS_LIMIT in the script.)

Ok! Didn't catch that.

I found another "issue" in src/ext/OpenBSD_malloc_Linux.c:

#else // !(!defined(BUILDING_FOR_TOR)) ... which seems odd. Maybe the script is confused by the #define at the start of the file and then #ifndef later ?

I mean "double negative" is right just kind of harder to parse. If it is an easy fix to detect double negative to be just "defined()", great else no biggy. Anyway, I'm being picky here just for documentation :P.

Trac:
Reviewer: N/A to dgoulet
Status: needs_revision to merge_ready

I think the src/ext/openbsd.. issue should be okay; I'm not planning to run this script on src/ext, since that isn't our code. We can improve the output there later if we want.

For the wide lines, I'll try to fix that one way or another when I merge, which I intend to do when we freeze, so that nothing runs into conflicts with this patch this week.

ok, fixed it all up and merged.

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

closed

added 48m of time spent

moved to tpo/core/tor#20168 (closed)