Run Clang-Tidy check for namespace comments

RESOLVED FIXED in Firefox 42

Status

RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: poiru, Assigned: poiru)

Tracking

Trunk
mozilla42

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
We should use the 'llvm-namespace-comment' check to fix our `// namespace foo` comments.
(Assignee)

Comment 1

3 years ago
Created attachment 8632665 [details] [diff] [review]
Fix and add missing namespace 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 2

3 years ago
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+
(Assignee)

Comment 4

3 years ago
(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
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
This patch incorrectly converted '// anonymous namespace' to '// namespace'; for example in ipc/ril/Ril.cpp.
(Assignee)

Comment 7

3 years ago
(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.

Updated

3 years ago
Depends on: 1245379

Updated

8 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.