Closed Bug 1182996 Opened 9 years ago Closed 9 years ago

Run Clang-Tidy check for namespace comments

Categories

(Developer Infrastructure :: Source Code Analysis, defect)

defect
Not set
normal

Tracking

(firefox42 fixed)

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: poiru, Assigned: poiru)

References

Details

Attachments

(1 file)

We should use the 'llvm-namespace-comment' check to fix our `// namespace foo` comments.
The bulk of this commit was generated by running:

  run-clang-tidy.py \
    -checks='-*,llvm-namespace-comment' \
    -header-filter=^/.../mozilla-central/.* \
    -fix
Attachment #8632665 - Flags: review?(ehsan)
Comment on attachment 8632665 [details] [diff] [review]
Fix and add missing namespace comments

Review of attachment 8632665 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me

Note that I am going to add a .clang-tidy file in bug 1158656.  I will make a note of adding llvm-namespace-comment there.

Also, I'm curious to know what your experience was like running clang-tidy.  Do you mind providing a list of the steps you took and what worked and what didn't, including your platform?  Thanks!
Attachment #8632665 - Flags: review?(ehsan) → review+
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #2)
> Also, I'm curious to know what your experience was like running clang-tidy. 
> Do you mind providing a list of the steps you took and what worked and what
> didn't, including your platform?  Thanks!

I used your patch in bug 904572 to generate a compilation database and then used the command in comment 1 to rewrite the whole tree. I'm on OS X, so it might be useful to run this on Linux (and Windows, if we can).

At the moment, running that command will fail due to conflicts in Hal.h and HalInternal.h (because of `namespace MOZ_HAL_NAMESPACE {`, where MOZ_HAL_NAMESPACE has different values in different translation units). I got rid of that locally to workaround it.

Additionally, clang-tidy doesn't do a good job if the namespace ends with a semicolon (`};`) or a block comment (`} /* foo */`). I'll probably look at fixing all three issues and upstreaming them.

The main issue is that clang-tidy will also touch 3rd pary code that we don't want to reformat. This is a larger issue, so I filed bug 1183143.
https://hg.mozilla.org/mozilla-central/rev/91d6e262b662
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
This patch incorrectly converted '// anonymous namespace' to '// namespace'; for example in ipc/ril/Ril.cpp.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #6)
> This patch incorrectly converted '// anonymous namespace' to '// namespace';
> for example in ipc/ril/Ril.cpp.

That's a popular way to mark anonymous namespaces (see e.g. Google style guide) so arguably it's correct. If you disagree, please ni? bsmedberg for a decision.
Depends on: 1245379
Product: Core → Firefox Build System
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.