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:
> 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.
================
Comment at: clang-tidy/ClangTidyOptions.h:216
@@ +215,3 @@
+/// HeaderFileExtensions.
+bool endWithHeaderFileExtensions(llvm::StringRef FileName,
+ llvm::StringRef HeaderFileExtensions);
----------------
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.
================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:78
@@ +77,3 @@
+static cl::opt<std::string>
+HeaderFileExtensions("header-file-extensions",
+ cl::desc("File extensions that regard as header file.\n"
----------------
alexfh wrote:
> hokein wrote:
> > alexfh wrote:
> > > We don't need a command-line flag for this. The regular way to configure
> > > clang-tidy is .clang-tidy files. However, if needed, check options can be
> > > configured from the command line via the `-config=` option.
> > From my understanding, it is a global option for header-file-extensions,
> > right? If we remove this command-line flag, there is only local
> > `header-file-extensions` option remained for particular checks.
> Both local and global options reside in `CheckOptions` and can be configured
> in the same way using either a .clang-tidy file or the -config= command-line
> option. The only difference between local and global options is that the name
> of the latter is not prefixed with the "CheckName.". Makes sense?
> Both local and global options reside in CheckOptions and can be configured in
> the same way using either a .clang-tidy file or the -config= command-line
> option. The only difference between local and global options is that the name
> of the latter is not prefixed with the "CheckName.". Makes sense?
Ah, thank you for that explanation. I learned something new today. ;-)
Repository:
rL LLVM
http://reviews.llvm.org/D16113
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
