shafik added a comment.
Herald added a subscriber: wangpc.
Herald added a project: All.
Is this still relevant? It looks like some of these changes made it in.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D23130/new/
https://reviews.llvm.org/D23130
_
alexfh added a comment.
In https://reviews.llvm.org/D23130#1037256, @bkramer wrote:
> I'd like to, but I don't know when I find time to rebase this thing after
> more than a year of waiting for review.
Sorry, it was just lost ¯\_(ツ)_/¯. I may be relying on pings from the other
side too much.
bkramer added a comment.
I'd like to, but I don't know when I find time to rebase this thing after more
than a year of waiting for review.
https://reviews.llvm.org/D23130
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org
alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: xazax.hun.
It looks like all concerns were resolved.
LG, if you still want to proceed with this patch.
https://reviews.llvm.org/D23130
__
malcolm.parsons added inline comments.
Comment at: clang-tidy/google/GoogleTidyModule.cpp:68
+CheckFactories.registerCheck(
+"google-global-names");
CheckFactories.registerCheck(
bkramer wrote:
> aaron.ballman wrote:
> > Given that this was shipp
bkramer added inline comments.
Comment at: clang-tidy/google/GlobalNamesCheck.cpp:90
+// extern "C" globals need to be in the global namespace.
+if (VDecl->isExternC())
+ return;
alexfh wrote:
> Is this already filtered-out by the matcher?
Nope.
==
bkramer updated this revision to Diff 80574.
bkramer marked 9 inline comments as done.
bkramer added a comment.
Herald added a subscriber: JDevlieghere.
- Moved external linkage check to matcher
- added msvcrt entry point check
- fixed comment typos.
https://reviews.llvm.org/D23130
Files:
cla
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added inline comments.
Comment at: clang-tidy/google/GlobalNamesCheck.cpp:62
+Result.SourceManager->getExpansionLoc(D->getLocStart( {
+ // unless that file is a header.
+ if (!utils::isSpelling
bkramer marked 2 inline comments as done.
bkramer added inline comments.
Comment at: test/clang-tidy/google-global-names.cpp:13-14
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'i' declared in the global
namespace
+extern int ii = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning:
aaron.ballman added inline comments.
Comment at: test/clang-tidy/google-global-names.cpp:13-14
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'i' declared in the global
namespace
+extern int ii = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'ii' declared in the global
namespa
malcolm.parsons added inline comments.
Comment at: clang-tidy/google/GlobalNamesCheck.cpp:96
+// main() should be in the global namespace.
+if (FDecl->isMain())
+ return;
Should `isMSVCRTEntryPoint()` be checked too?
https://reviews.llvm.org/D23130
bkramer marked 2 inline comments as done.
bkramer added a comment.
In https://reviews.llvm.org/D23130#589643, @alexfh wrote:
> > and generally frowned upon in many codebases (e.g. LLVM)
>
> Should it still be a part of google/? The old check was enforcing a part of
> the Google C++ style guide,
bkramer updated this revision to Diff 77410.
bkramer added a comment.
Add extern "C++" test case.
https://reviews.llvm.org/D23130
Files:
clang-tidy/google/CMakeLists.txt
clang-tidy/google/GlobalNamesCheck.cpp
clang-tidy/google/GlobalNamesCheck.h
clang-tidy/google/GlobalNamesInHeadersChe
alexfh added a comment.
> and generally frowned upon in many codebases (e.g. LLVM)
Should it still be a part of google/? The old check was enforcing a part of the
Google C++ style guide, but the new one seems to be somewhat broader. Am I
mistaken?
https://reviews.llvm.org/D23130
__
aaron.ballman added inline comments.
Comment at: clang-tidy/google/GlobalNamesCheck.cpp:77
+}
+diag(
+D->getLocStart(),
Is this formatting that clang-format generates?
Comment at: test/clang-tidy/google-global-names.cpp:13-14
+/
bkramer added a comment.
In https://reviews.llvm.org/D23130#588681, @alexfh wrote:
> Benjamin, what's the plan here?
I still think this check is useful, particularly for LLVM. I also don't think
any of the existing review comments still apply or have ever applied in the
first place, so I reba
bkramer updated this revision to Diff 77216.
bkramer added a comment.
Herald added subscribers: modocache, mgorny.
Update to head
https://reviews.llvm.org/D23130
Files:
clang-tidy/google/CMakeLists.txt
clang-tidy/google/GlobalNamesCheck.cpp
clang-tidy/google/GlobalNamesCheck.h
clang-tid
alexfh added a comment.
Benjamin, what's the plan here?
https://reviews.llvm.org/D23130
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
aaron.ballman added inline comments.
Comment at: test/clang-tidy/google-global-names.cpp:13-14
@@ +12,4 @@
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'i' declared in the global
namespace
+extern int ii = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'ii' declared in the glob
bkramer added inline comments.
Comment at: test/clang-tidy/google-global-names.cpp:13-14
@@ +12,4 @@
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'i' declared in the global
namespace
+extern int ii = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'ii' declared in the global
na
aaron.ballman added a comment.
For cases where the external linkage is desired, how would you go about
silencing this diagnostic?
Comment at: test/clang-tidy/google-global-names.cpp:13-14
@@ +12,4 @@
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'i' declared in the global
names
bkramer added a comment.
In https://reviews.llvm.org/D23130#505769, @alexfh wrote:
> Could you run the check on LLVM and post a summary of results?
I have a full list at https://reviews.llvm.org/P7085. It's huge.
https://reviews.llvm.org/D23130
_
bkramer updated this revision to Diff 66800.
bkramer added a comment.
Address review comments.
https://reviews.llvm.org/D23130
Files:
clang-tidy/google/CMakeLists.txt
clang-tidy/google/GlobalNamesCheck.cpp
clang-tidy/google/GlobalNamesCheck.h
clang-tidy/google/GlobalNamesInHeadersCheck.
alexfh added a comment.
Could you run the check on LLVM and post a summary of results?
Comment at: clang-tidy/google/GlobalNamesCheck.cpp:90
@@ +89,3 @@
+// extern "C" globals need to be in the global namespace.
+if (VDecl->isExternC())
+ return;
Is
bkramer updated this revision to Diff 66792.
bkramer added a comment.
- Merge google-global-names-in-headers and misc-global-namespace into a new
check google-global-names.
https://reviews.llvm.org/D23130
Files:
clang-tidy/google/CMakeLists.txt
clang-tidy/google/GlobalNamesCheck.cpp
clan
aaron.ballman added inline comments.
Comment at: clang-tidy/misc/GlobalNamespaceCheck.cpp:72
@@ +71,3 @@
+ "move it into a namespace or give it "
+ "internal linkage to avoid ODR conflicts")
+ << MatchedDecl;
bkramer added a comment.
In https://reviews.llvm.org/D23130#505290, @alexfh wrote:
> And the second difference is that it is limited to definitions, but I don't
> yet understand, why it is important, since declarations in the global
> namespace (except for using declarations, typedefs, etc.) wi
Prazek added inline comments.
Comment at: clang-tidy/misc/GlobalNamespaceCheck.cpp:46
@@ +45,3 @@
+// extern "C" globals need to be in the global namespace.
+if (VDecl->isExternC())
+ return;
I think it would be better to check it in matcher.
I see th
alexfh added a comment.
In https://reviews.llvm.org/D23130#505242, @bkramer wrote:
> DefinitionsInHeaders is tackling a different problem. IMO
> DefinitionsInHeaders is something that should be on by default everywhere,
> while this check for definitions in the global namespace is more of a cod
bkramer added a comment.
DefinitionsInHeaders is tackling a different problem. IMO DefinitionsInHeaders
is something that should be on by default everywhere, while this check for
definitions in the global namespace is more of a coding style issue.
GlobalNamesInHeaders is a bit of a misnomer, it
On Wed, Aug 3, 2016 at 5:17 PM, Alexander Kornienko wrote:
> alexfh added a comment.
>
> Do we want to merge this check with google/GlobalNamesInHeadersCheck.cpp that
> targets a similar set of issues?
We also have misc/DefinitionsInHeadersCheck.cpp which is likely similar.
~Aaron
>
>
> Reposi
alexfh added a comment.
Do we want to merge this check with google/GlobalNamesInHeadersCheck.cpp that
targets a similar set of issues?
Repository:
rL LLVM
https://reviews.llvm.org/D23130
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ht
bkramer updated this revision to Diff 66716.
bkramer added a comment.
Add relnote.
https://reviews.llvm.org/D23130
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/GlobalNamespaceCheck.cpp
clang-tidy/misc/GlobalNamespaceCheck.h
clang-tidy/misc/MiscTidyModule.cpp
docs/ReleaseNotes
33 matches
Mail list logo
