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

[Dmitri Gribenko]

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


================
Comment at: include/clang/AST/ASTContext.h:442
@@ -436,1 +441,3 @@
 
+  const CommentOptions& getCommentOpts() const { return CommentOpts; }
+
----------------
The space should be before the ampersand.

================
Comment at: lib/AST/CommentCommandTraits.cpp:21
@@ -19,2 +20,3 @@
     NextID(llvm::array_lengthof(Commands)), Allocator(Allocator)
-{ }
+{
+  registerBlockCommands(CommentOptions.BlockCommandNames);
----------------
The brace should be on the previous line.

================
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);
----------------
According to coding guidelines, function name should start with a lowercase letter.

================
Comment at: include/clang/Basic/CommentOptions.h:25
@@ +24,3 @@
+struct CommentOptions {
+  typedef llvm::SmallVector<llvm::StringRef, 4> BlockCommandNamesTy;
+
----------------
'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).

================
Comment at: include/clang/Basic/CommentOptions.h:27
@@ +26,3 @@
+
+  /// \brief Strings to treat as names of block commands in comments.
+  BlockCommandNamesTy BlockCommandNames;
----------------
This comment might suggest that we should put a complete command here, including the backslash.

A better one: command names to treat as ...

================
Comment at: lib/AST/CommentCommandTraits.cpp:60
@@ +59,3 @@
+void CommandTraits::registerBlockCommands(
+  const CommentOptions::BlockCommandNamesTy &BlockCommandNames) {
+  for (CommentOptions::BlockCommandNamesTy::const_iterator I =
----------------
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.

================
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));
+}
----------------
This saves StringRefs to std::strings that are going to be destroyed on the next line, doesn't it?

================
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,
----------------
Space should be before the ampersand.

================
Comment at: unittests/AST/CommentLexer.cpp:457
@@ +456,3 @@
+// Custom registered block command.
+TEST_F(CommentLexerTest, DoxygenCommentRegisterBlockCommand) {
+  const char *Source = "/// \\NewBlockCommand Aaa.\n";
----------------
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)

================
Comment at: unittests/AST/CommentLexer.cpp:484
@@ +483,3 @@
+// Multiple custom registered block commands.
+TEST_F(CommentLexerTest, DoxygenCommentRegisterMultipleBlockCommands) {
+  const char *Source =
----------------
s/DoxygenCommentRegisterMultipleBlockCommands/RegisterMultipleCustomBlockCommands/
This test becomes redundant if we change CommentTraits to register only a single command.  Or maybe not, I don't mind if you leave it.  This has an interesting case of two custom commands following each other.  This is not different from builtin commands at this moment, but might change in future.


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