[PATCH] D15710: [clang-tidy] Add non-inline function definition and variable definition check in header files.

Alexander Kornienko via cfe-commits cfe-commits at lists.llvm.org
Mon Jan 4 07:29:37 PST 2016


alexfh added inline comments.

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:25
@@ +24,3 @@
+  SourceLocation ExpansionLoc = SM.getExpansionLoc(Node.getLocStart());
+  StringRef Filename = SM.getFilename(ExpansionLoc);
+  return Filename.endswith(".h") || Filename.endswith(".hh") ||
----------------
alexfh wrote:
> hokein wrote:
> > aaron.ballman wrote:
> > > > We're looking at the problem from different angles. My view is that a reasonable file naming convention (which at least makes interface header files, textual headers and main files distinguishable) is a widespread enough practice, and the benefits it brings outweigh the costs of enforcing it. However, the opposite point of view also has its right to exist, so we need a solution that fits both ;)
> > > >> Perhaps another solution to this is use isInMainFile() || usesHeaderFileExtension().
> > > >You probably meant !isInMainFile() || usesHeaderFileExtension(). I guess, that will work for us. We could also make the list of header file extensions (or a regular expression pattern for header files) configurable, so that the usesHeaderFileExtension() part could be disabled, if needed.
> > > 
> > > Oops, you are correct, I meant !isInMainFile(). :-) I definitely agree that we should make the header file extensions configurable. Would it make sense if this were a global option that any checker can use? We have 3-4 other checkers that care about header files as well, and it would be nice if they all behaved consistently without the user having to write a lot of options for each checker.
> > I'm :+1 on making header file extensions configurable. I think we can do that in a new patch.
> Having to configure this in a single place would be convenient. OTOH, I can imagine corner-cases, where a single option would be undesired. One thing we could do is to allow global options that can be overridden for each check. Something along the lines of (modulo tests):
> 
> ```
>   std::string OptionsView::get(StringRef LocalName, std::string Default) const {
> -   const auto &Iter = CheckOptions.find(NamePrefix + LocalName.str());
> +   auto Iter = CheckOptions.find(NamePrefix + LocalName.str());
>     if (Iter != CheckOptions.end())
>       return Iter->second;
> +   // Fallback to global setting, if present.
> +   Iter = CheckOptions.find(LocalName.str());
> +   if (Iter != CheckOptions.end())
> +     return Iter->second;
>     return Default;
>   }
> ```
> 
> Alternatively, we could add another method (e.g. `getLocalOrGlobal`) to make the usage of the global setting explicit.
> 
> What do you think?
This should read "Having a way to configure this in a single place ..."


http://reviews.llvm.org/D15710





More information about the cfe-commits mailing list