[clang-tidy] minor bug fix to AbseilMatcher.h

[hugoeg]

Summary

This missing directory is not yet released, but is causing some problems internally. It's gonna be released eventually and received permission to include it here. This matcher will also be periodically updated by my team as we have more releases and or problems internally.

[hugoeg]

trying to follow style conventions, please correct me if I am wrong

[ioeric]

AbseilMatcher.h

52 ↗

(On Diff #164091)

did you run clang-format on this? It looks a bit strange.

[hugoeg]

AbseilMatcher.h

52 ↗

(On Diff #164091)

yea, clang-format was vastly different to what was originally there, so I tried to follow those conventions, should I go back to clang-format?

[hugoeg]

clang-format

[hugoeg]

also, I don't have commit access, so if you could land this for me when you get the chance, I'd appreciate it

[ioeric]

AbseilMatcher.h

52 ↗

(On Diff #164091)

Let's go with clang-format ;)

[lebedev.ri]

Please don't forget to set the appropriate repository when creating the differential,
so phabricator can automagically subscribe the right -commits mailing list.

[kbobyrev]

Hi @hugoeg! Thank you for the update. Eric was too fast for me to catch up :)

I'm not sure about the _correctness_ and maintainability; I would probably suggest something like warning: absl/flags is reserved for future use for the part of Abseil which is not released yet (since it's hard to follow and might be confusing). Also, I'm not sure about hard-coding the library names in general: some pieces of Abseil can become deprecated, other ones are added eventually (like "flags" in this case). Would matching absl/ substring be enough here? I can't see any good reason users would call their directories absl/ (and not expect Clang-Tidy to produce some warnings :) and that would make it easier for the Abseil team, because you wouldn't have to update hard-coded library names in a specific Clang-Tidy file each release (which is very easy to forget). That would also simplify the code a lot, although it's not a concern for me.

@ioeric, @hugoeg what do you think?

Also, if you manually upload diff, please consider adding -U999999 to your Git or SVN invocation (see LLVM's Phabricator guide for reference). That would help reviewers to get the context of the file which is up for a review.

clang-tools-extra/trunk/clang-tidy/abseil/AbseilMatcher.h

AST_POLYMORPHIC_MATCHER(
   // Determine whether filepath contains "absl/[absl-library]" substring, where
   // [absl-library] is AbseilLibraries list entry.
   StringRef Path = FileEntry->getName();
   const static llvm::SmallString<5> AbslPrefix("absl/");
   size_t PrefixPosition = Path.find(AbslPrefix);
   if (PrefixPosition == StringRef::npos)
     return false;
   Path = Path.drop_front(PrefixPosition + AbslPrefix.size());
-  static const char *AbseilLibraries[] = {
-      "algorithm",       "base", "container", "debugging",
-      "memory",          "meta", "numeric",   "strings",
-      "synchronization", "time", "types",     "utility"};
+  static const char *AbseilLibraries[] = {"algorithm",
+                                          "base",
+                                          "container",
+                                          "debugging",
+                                          "flags"
+                                          "memory",
+                                          "meta",
+                                          "numeric",
+                                          "strings",
+                                          "synchronization",
+                                          "time",
+                                          "types",
+                                          "utility"};
   return std::any_of(
       std::begin(AbseilLibraries), std::end(AbseilLibraries),
       [&](const char *Library) { return Path.startswith(Library); });
 }
 
 } // namespace ast_matchers
 } // namespace clang