> 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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
