[cfe-commits] [PATCH] Add CommentOptions, allow specifying custom comment block commands

[Ben Gertzfield]

  > If we make CommentOptions a member of LanguageOptions, most of this patch vanishes.

  Sure, I can do that. I wasn't sure exactly what belonged in language options (since this has nothing to do with any particular programming language), which is why I made it separate.


================
Comment at: include/clang/Basic/CommentOptions.h:25
@@ +24,3 @@
+struct CommentOptions {
+  typedef llvm::SmallVector<llvm::StringRef, 4> BlockCommandNamesTy;
+
----------------
Dmitri Gribenko wrote:
> '4' can be safely changed to '1', since most of the time there will be no commands here.
> 
> Or even use a std::vector since otherwise it is not clear who owns memory for StringRefs (when CommentOptions is created, ASTContext is not yet created, so we can not allocate memory there).
I see, thanks for clarifying. Should I just use std::vector<std::string> then?


================
Comment at: lib/AST/ASTContext.cpp:605-606
@@ -604,2 +604,4 @@
 
-ASTContext::ASTContext(LangOptions& LOpts, SourceManager &SM,
+ASTContext::ASTContext(LangOptions& LOpts,
+                       CommentOptions& COpts,
+                       SourceManager &SM,
----------------
Dmitri Gribenko wrote:
> Space should be before the ampersand.
OK. I was just copying the style of ##LangOptions& LOpts##.

================
Comment at: lib/AST/CommentCommandTraits.cpp:37
@@ -33,3 +36,3 @@
 
-const CommandInfo *CommandTraits::registerUnknownCommand(StringRef CommandName) {
+CommandInfo *CommandTraits::CreateCommandInfoWithName(StringRef CommandName) {
   char *Name = Allocator.Allocate<char>(CommandName.size() + 1);
----------------
Dmitri Gribenko wrote:
> According to coding guidelines, function name should start with a lowercase letter.
Gotcha.


================
Comment at: lib/AST/CommentCommandTraits.cpp:60
@@ +59,3 @@
+void CommandTraits::registerBlockCommands(
+  const CommentOptions::BlockCommandNamesTy &BlockCommandNames) {
+  for (CommentOptions::BlockCommandNamesTy::const_iterator I =
----------------
Dmitri Gribenko wrote:
> ArrayRef, please.
> 
> Actually, I think that a cleaner interface would be to register only one command.  There is only one caller of this that needs this 'register in bulk' functionality, and we can move the loop there.
OK.


================
Comment at: lib/Frontend/CompilerInvocation.cpp:306-308
@@ +305,5 @@
+    Args.getAllArgValues(OPT_fcomment_block_commands);
+  std::copy(
+    BlockCommandNames.begin(), BlockCommandNames.end(),
+    std::back_inserter(Opts.BlockCommandNames));
+}
----------------
Dmitri Gribenko wrote:
> This saves StringRefs to std::strings that are going to be destroyed on the next line, doesn't it?
Yeah, that's wrong. Thanks for clarifying how StringRefs work.


================
Comment at: unittests/AST/CommentLexer.cpp:457
@@ +456,3 @@
+// Custom registered block command.
+TEST_F(CommentLexerTest, DoxygenCommentRegisterBlockCommand) {
+  const char *Source = "/// \\NewBlockCommand Aaa.\n";
----------------
Dmitri Gribenko wrote:
> Since this is not testing Doxygen-specific features,
> s/DoxygenCommentRegisterBlockCommand/RegisterCustomBlockCommand/
> 
> The comment can be deleted then.  (It is always better to have explanations in the names in code than in comments)
Sounds good.



http://llvm-reviews.chandlerc.com/D272