[PATCH] D50542: [clang-tidy] Add abseil-no-internal-deps check

Jonas Toth via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Aug 20 07:10:40 PDT 2018


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 causes them to overlook it.



================
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:
> > JonasToth wrote:
> > > hugoeg wrote:
> > > > hokein wrote:
> > > > > nit: please make sure the code follow LLVM code style, even for test code :)
> > > > what is this in reference too?
> > > > Will the test still work if I wrap the CHECK MESSAGE lines?
> > > CHECK-MESSAGE can be on one line, even if its longer (that is common in the clang-tidy tests).
> > > 
> > > But dont use many empty lines and respect naming conventions and run clang-format over the code (except there is a valid reason that the formatting would infer with the tested logic).
> > Do my function names have to be verbs, they're not doing anything.
> > 
> > I could rename InternalFunction to something like InternallyProcessString annd StringFunction to process String
> > Would this be preferred?
> It helps if you somehow show the "topic of the function". But I wrote some tests, that did not strictly follow and they passed review too ;)
> 
> Foo is just too generic, sth like `DirectAccess`, `FriendUsage`, `OpeningNamespace` or so is already telling and I guess good enough :)
I think the names are ok.


https://reviews.llvm.org/D50542





More information about the cfe-commits mailing list