alexfh added inline comments. ================ Comment at: clang-tidy/ClangTidyOptions.h:216 @@ +215,3 @@ +/// HeaderFileExtensions. +bool endWithHeaderFileExtensions(llvm::StringRef FileName, + llvm::StringRef HeaderFileExtensions); ---------------- aaron.ballman wrote: > aaron.ballman wrote: > > alexfh wrote: > > > hokein wrote: > > > > aaron.ballman wrote: > > > > > alexfh wrote: > > > > > > This function doesn't belong here. I'm also not sure we need this > > > > > > function at all. First, it's ineffective to split the string > > > > > > containing the list of extensions each time. Second, if we store a > > > > > > set of extensions, then we can just search for the actual file > > > > > > extension in this set. > > > > > endsWithHeaderFileExtension instead? However, given that uses of this > > > > > all start with a SourceLocation, I wonder if that makes for a cleaner > > > > > API: isLocInHeaderFile(SourceLocation, <Extensions>); > > > > > > > > > > Also, how does this work if I want to include an extension-less file > > > > > as the header file "extension?" It would be plausible if the > > > > > extensions were passed in as a list, but as it stands it doesn't seem > > > > > possible without weird conventions like leaving a blank in the list > > > > > (e.g., `.h,,.hpp`), which seems error-prone. > > > > > > > > > > Also, I'm not certain what I can pass in. The documentation should be > > > > > updated to state whether these extensions are intended to include the > > > > > ".". > > > > > endsWithHeaderFileExtension instead? However, given that uses of this > > > > > all start with a SourceLocation, I wonder if that makes for a cleaner > > > > > API: isLocInHeaderFile(SourceLocation, <Extensions>); > > > > > > > > Using `SourceLocation` only is not enough to retrieve the belonging > > > > file name (we need `SourceManager` too). > > > > > > > > >Also, how does this work if I want to include an extension-less file > > > > >as the header file "extension?" It would be plausible if the > > > > >extensions were passed in as a list, but as it stands it doesn't seem > > > > >possible without weird conventions like leaving a blank in the list > > > > >(e.g., .h,,.hpp), which seems error-prone. > > > > > > > > Yeah, for extensions-less header file, you can pass the string like > > > > `.h,,.hpp`, which is a bit of weird. Do you have a better idea here? > > > > Passing a string into `header-file-extensions` seems the most > > > > reasonable choice. > > > > > > > `isLocInHeaderFile(SourceLocation, ...)` is a nice idea, but we'd need to > > > be more specific: either `isExpansionLocInHeaderFile(SourceLoc, ...)` or > > > `isSpellingLocInHeaderFile(SourceLoc, ...)` (or both). > > > Yeah, for extensions-less header file, you can pass the string like > > > .h,,.hpp, which is a bit of weird. Do you have a better idea here? > > > Passing a string into header-file-extensions seems the most reasonable > > > choice. > > > > I thought those user configurations from the command line were in YAML or > > JSON format, those both have the notion of lists, don't they? I would > > imagine this would take a SmallVectorImpl<StringRef/std::string> for the > > list of extensions. > > isLocInHeaderFile(SourceLocation, ...) is a nice idea, but we'd need to be > > more specific: either isExpansionLocInHeaderFile(SourceLoc, ...) or > > isSpellingLocInHeaderFile(SourceLoc, ...) (or both). > > That's true, and I would think both are reasonable to add. I rather prefer > that as an API instead of passing around file names as strings, personally. User configurations are stored in YAML, however, CheckOptions is a map of strings to strings, so we can't use YAML lists to represent lists of extensions. I personally see nothing wrong with `",.h,.hh,.hpp"` for example, to represent the list of extensions (<empty> being the first one makes it somewhat stand out and provided there's a good documentation, this shouldn't be too confusing).
Repository: rL LLVM http://reviews.llvm.org/D16113 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
