aaron.ballman added inline comments. ================ Comment at: clang-tidy/ClangTidyOptions.h:216 @@ +215,3 @@ +/// HeaderFileExtensions. +bool endWithHeaderFileExtensions(llvm::StringRef FileName, + llvm::StringRef HeaderFileExtensions); ---------------- alexfh wrote: > 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). > 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).
I find it to be more clever than intuitive. If it were not user-facing, I would be less concerned. I don't think users should have to read documentation to figure out the *syntax* of how to pass options if we can at all avoid it. ;-) Regardless, I would like to separate the two concepts -- there's the way we expose the option to the users, and there's our internal APIs that we call. I don't think the internal API has to take such an awkward thing directly just because the user-facing option has to be that way currently. 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
