JonasToth added a comment.
Ok. Then we can commit for you guys, no problem :)
Am 29.08.2018 um 16:34 schrieb Hugo Gonzalez via Phabricator:
> hugoeg added a comment.
>
> In https://reviews.llvm.org/D50542#1217517, @JonasToth wrote:
>
>> Committed r340928. Thanks very much!
>>
>> Will you (and
hugoeg added a comment.
In https://reviews.llvm.org/D50542#1217517, @JonasToth wrote:
> Committed r340928. Thanks very much!
>
> Will you (and your team) upstream even more abseil checks? I think it might
> be reasonable to ask for commit rights.
We will likely not be up streaming more in the
This revision was automatically updated to reflect the committed changes.
Closed by commit rL340928: [clang-tidy] Add abseil-no-internal-dependencies
check (authored by JonasToth, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D50542?v
JonasToth added a comment.
Committed r340928. Thanks very much!
Will you (and your team) upstream even more abseil checks? I think it might be
reasonable to ask for commit rights.
https://reviews.llvm.org/D50542
___
cfe-commits mailing list
cfe-co
hugoeg added a comment.
In https://reviews.llvm.org/D50542#1217515, @JonasToth wrote:
> In https://reviews.llvm.org/D50542#1217511, @hugoeg wrote:
>
> > nit comment fixed
>
>
> DO you have commit rights or shall i commit for you?
I do not have commit rights so I would appreciate it if you would
JonasToth added a comment.
In https://reviews.llvm.org/D50542#1217511, @hugoeg wrote:
> nit comment fixed
DO you have commit rights or shall i commit for you?
https://reviews.llvm.org/D50542
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
hugoeg updated this revision to Diff 163083.
hugoeg marked an inline comment as done.
hugoeg added a comment.
nit comment fixed
https://reviews.llvm.org/D50542
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoInternalDependenciesCheck.cpp
hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.
LGTM with a nit, thanks for your work.
Comment at: clang-tidy/abseil/NoInternalDependenciesCheck.h:20
+/// Finds instances where the user depends on internal details and warn
hugoeg updated this revision to Diff 162921.
hugoeg added a comment.
renamed check as suggested
https://reviews.llvm.org/D50542
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoInternalDependenciesCheck.cpp
clang-tidy/abseil/NoInternalDe
hugoeg updated this revision to Diff 162895.
hugoeg marked 3 inline comments as done.
hugoeg added a comment.
fixed issues outlines in comments
https://reviews.llvm.org/D50542
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoInternalDepsCh
Eugene.Zelenko added a comment.
I still thik will be good idea to rename check (deps -> dependencies).
https://reviews.llvm.org/D50542
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
hugoeg marked an inline comment as done.
hugoeg added inline comments.
Comment at: test/clang-tidy/Inputs/absl/external-file.h:1
+void DirectAcess2() { absl::strings_internal::InternalFunction(); }
+
hokein wrote:
> The file can not self-compile, and we should ma
hokein added inline comments.
Comment at: test/clang-tidy/Inputs/absl/external-file.h:1
+void DirectAcess2() { absl::strings_internal::InternalFunction(); }
+
The file can not self-compile, and we should make it compilable.
And put the newly-added code at the en
hugoeg updated this revision to Diff 162864.
hugoeg marked 2 inline comments as done.
hugoeg added a comment.
made some major updates after no-namespace landed
https://reviews.llvm.org/D50542
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/
kbobyrev added inline comments.
Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:24
+ auto &SourceManager = Finder->getASTContext().getSourceManager();
+ SourceLocation loc = Node.getBeginLoc();
+ if (loc.isInvalid())
I think @hokein's comment wasn't addr
hokein added a comment.
Please rebase your patch, since the absl matcher patch is submitted. Thanks.
Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:8
+#include "absl/external-file.h"
+// CHECK: absl/external-file.h:1:23: warning: do not reference any 'internal'
namesp
hugoeg added inline comments.
Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:8
+#include "absl/external-file.h"
+// CHECK: absl/external-file.h:1:23: warning: do not reference any 'internal'
namespaces; those implementation details are reserved to Abseil
[abseil-no-int
hugoeg updated this revision to Diff 162379.
hugoeg marked 2 inline comments as done.
hugoeg added a comment.
fixed comments
https://reviews.llvm.org/D50542
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoInternalDepsCheck.cpp
clang-tid
hokein added inline comments.
Comment at: docs/clang-tidy/checks/abseil-no-internal-deps.rst:17
+
+ absl::strings_internal::foo();
+ class foo {
I think this line is also triggered the warning?
Comment at: test/clang-tidy/abseil-no-internal-d
hugoeg added a comment.
Ok , so I've clean some stuff up, I'm currently working with @deannagarcia to
get https://reviews.llvm.org/D50580 submitted with the match I implemented for
us first, then will adjust this as necessary
https://reviews.llvm.org/D50542
_
hugoeg updated this revision to Diff 161714.
hugoeg marked an inline comment as done.
hugoeg added a comment.
All comments have been address and documentation has been added to matcher
https://reviews.llvm.org/D50542
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists
hokein added a subscriber: deannagarcia.
hokein added a comment.
Your patch seems have some comments unaddressed but you marked done.
About the abseil matcher stuff, you and @deannagarcia have an overlap, maybe
just wait for https://reviews.llvm.org/D50580 to submit, and reuse the matcher
in th
hugoeg updated this revision to Diff 161550.
hugoeg added a comment.
refined test to include some more cases as a result of the new matcher
https://reviews.llvm.org/D50542
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoInternalDepsCheck.
hugoeg marked an inline comment as done.
hugoeg added inline comments.
Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:20
+
+bool IsInAbseilFile(const SourceManager& manager, SourceLocation loc){
+ if (loc.isInvalid()) {
hokein wrote:
> hugoeg wrote:
> > h
hugoeg updated this revision to Diff 161494.
hugoeg marked 5 inline comments as done.
hugoeg added a comment.
corrections applied
https://reviews.llvm.org/D50542
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoInternalDepsCheck.cpp
clan
hokein added inline comments.
Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:20
+
+bool IsInAbseilFile(const SourceManager& manager, SourceLocation loc){
+ if (loc.isInvalid()) {
hugoeg wrote:
> hokein wrote:
> > I think we can make it as an ASTMatcher in
hugoeg updated this revision to Diff 161485.
hugoeg added a comment.
fixed some minor things in the test
https://reviews.llvm.org/D50542
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoInternalDepsCheck.cpp
clang-tidy/abseil/NoInternalD
JonasToth added a comment.
In https://reviews.llvm.org/D50542#1205880, @hugoeg wrote:
> Anymore changes I should make or issues I should be aware of?
From my side not!
You still have unresolved comments that cause the revision to not be `Ready To
Review` for the main reviewers, maybe that caus
hugoeg added a comment.
Anymore changes I should make or issues I should be aware of?
https://reviews.llvm.org/D50542
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
hugoeg added inline comments.
Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:20
+
+bool IsInAbseilFile(const SourceManager& manager, SourceLocation loc){
+ if (loc.isInvalid()) {
hokein wrote:
> I think we can make it as an ASTMatcher instead of a functio
hugoeg updated this revision to Diff 161263.
hugoeg marked 5 inline comments as done.
hugoeg added a comment.
applied corrections form comments
https://reviews.llvm.org/D50542
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoInternalDepsCh
hokein added inline comments.
Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:20
+
+bool IsInAbseilFile(const SourceManager& manager, SourceLocation loc){
+ if (loc.isInvalid()) {
I think we can make it as an ASTMatcher instead of a function like:
```
AST
JonasToth added inline comments.
Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:21
+bool IsInAbseilFile(const SourceManager& manager, SourceLocation loc){
+ if (loc.isInvalid()) {
+return false;
You can elide the braces for single stmt ifs
=
hugoeg updated this revision to Diff 161130.
hugoeg added a comment.
added IsInAbseilFile Matcher
https://reviews.llvm.org/D50542
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoInternalDepsCheck.cpp
clang-tidy/abseil/NoInternalDepsChec
hugoeg added inline comments.
Comment at: test/clang-tidy/abseil-fake-declarations.h:1
+namespace std {
+struct string {
hokein wrote:
> I'd expect this header file is used as as a real absl library file:
>
> * create an `absl` directory in `test/clang-tidy/Inpu
hokein added a comment.
@hugoeg, it looks like you are waiting for review, but this patch doesn't
include the `IsExpansionInAbseilHeader` matcher. What's your plan of it?
Comment at: test/clang-tidy/abseil-fake-declarations.h:1
+namespace std {
+struct string {
---
hugoeg updated this revision to Diff 160562.
hugoeg marked an inline comment as done.
https://reviews.llvm.org/D50542
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoInternalDepsCheck.cpp
clang-tidy/abseil/NoInternalDepsCheck.h
docs/Rel
JonasToth added inline comments.
Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:5
+
+void DirectAcess(){
+ absl::strings_internal::InternalFunction();
Please run clang-format over the test code as well. The braces here in
`FriendUsage` miss a space.
hugoeg updated this revision to Diff 160432.
hugoeg marked an inline comment as done.
hugoeg added a comment.
applied corrections from comments
https://reviews.llvm.org/D50542
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoInternalDepsCh
JonasToth added inline comments.
Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:2
+// RUN: %check_clang_tidy %s abseil-no-internal-deps %t
+
+
hugoeg wrote:
> JonasToth wrote:
> > hugoeg wrote:
> > > hokein wrote:
> > > > nit: please make sure the code f
hugoeg updated this revision to Diff 160380.
hugoeg marked 3 inline comments as done.
https://reviews.llvm.org/D50542
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoInternalDepsCheck.cpp
clang-tidy/abseil/NoInternalDepsCheck.h
docs/Rel
hugoeg added inline comments.
Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:2
+// RUN: %check_clang_tidy %s abseil-no-internal-deps %t
+
+
JonasToth wrote:
> hugoeg wrote:
> > hokein wrote:
> > > nit: please make sure the code follow LLVM code style, ev
hugoeg updated this revision to Diff 160371.
https://reviews.llvm.org/D50542
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoInternalDepsCheck.cpp
clang-tidy/abseil/NoInternalDepsCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/ab
JonasToth added inline comments.
Comment at: clang-tidy/abseil/NoInternalDepsCheck.h:21
+/// against doing so. This check should not be run on internal Abseil files or
+///Abseil source code.
+///
double blank
Comment at: docs/clang-tidy/ch
JonasToth added inline comments.
Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:2
+// RUN: %check_clang_tidy %s abseil-no-internal-deps %t
+
+
hugoeg wrote:
> hokein wrote:
> > nit: please make sure the code follow LLVM code style, even for test code :)
hugoeg updated this revision to Diff 160363.
hugoeg added a comment.
most corrections from comments have been applied
https://reviews.llvm.org/D50542
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoInternalDepsCheck.cpp
clang-tidy/absei
hugoeg marked 8 inline comments as done.
hugoeg added inline comments.
Comment at: test/clang-tidy/abseil-no-internal-deps.cpp:2
+// RUN: %check_clang_tidy %s abseil-no-internal-deps %t
+
+
hokein wrote:
> nit: please make sure the code follow LLVM code style, ev
hokein added inline comments.
Comment at: docs/clang-tidy/checks/abseil-no-internal-deps.rst:6
+
+Gives a warning if code using Abseil depends on internal details. If something
is in a namespace or filename/path that includes the word “internal”, code is
not allowed to depend u
aaron.ballman added inline comments.
Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:24
+
+ // TODO(hugoeg): refactor matcher to be configurable or just match on any
internal access from outside the enclosing namespace.
+
JonasToth wrote:
> Nit: This co
JonasToth added a comment.
No concerns except the small nits from my side.
Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:24
+
+ // TODO(hugoeg): refactor matcher to be configurable or just match on any
internal access from outside the enclosing namespace.
+
hugoeg updated this revision to Diff 160168.
hugoeg added a comment.
corrections from comments applied
https://reviews.llvm.org/D50542
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoInternalDepsCheck.cpp
clang-tidy/abseil/NoInternalDep
Eugene.Zelenko added inline comments.
Comment at: docs/ReleaseNotes.rst:58
+
+ Warns Abseil users if they attempt to depend on internal details.
I think will be good idea to omit user, and just refer to code which depend on
internal details. Please synchron
JonasToth added a comment.
Thank you for your first contribution to LLVM btw :)
Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:24
+
+ TODO(hugoeg): refactor matcher to be configurable or just match on any
internal access from outside the enclosing namespace.
+
--
hugoeg updated this revision to Diff 160124.
https://reviews.llvm.org/D50542
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoInternalDepsCheck.cpp
clang-tidy/abseil/NoInternalDepsCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/ab
JonasToth added inline comments.
Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:24
+
+ Finder->addMatcher(
+ nestedNameSpecifierLoc(loc(specifiesNamespace(namespaceDecl(
hugoeg wrote:
> JonasToth wrote:
> > Actually that one is generally useful. Acce
hugoeg added inline comments.
Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:24
+
+ Finder->addMatcher(
+ nestedNameSpecifierLoc(loc(specifiesNamespace(namespaceDecl(
JonasToth wrote:
> Actually that one is generally useful. Accessing the `foo::inter
hugoeg updated this revision to Diff 160112.
hugoeg marked 10 inline comments as done.
hugoeg added a comment.
Applied corrections from first round comments
https://reviews.llvm.org/D50542
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/NoI
alexfh added inline comments.
Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:38
+ "depends upon internal implementation details, which violates the "
+ "abseil compatibilty guidelines. These can be found at "
+ "https://abseil.io/about/compatibility";);
-
JonasToth added inline comments.
Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:24
+
+ Finder->addMatcher(
+ nestedNameSpecifierLoc(loc(specifiesNamespace(namespaceDecl(
Actually that one is generally useful. Accessing the `foo::internal` from
outsi
59 matches
Mail list logo
