[cfe-commits] r164892 - in /cfe/trunk: include/clang/AST/ASTContext.h include/clang/AST/CommentSema.h include/clang/AST/RawCommentList.h include/clang/Lex/Preprocessor.h lib/AST/ASTContext.cpp lib/AST/CommentSema.cpp lib/AST/RawCommentList.cpp li

[Alexander Kornienko]

On Mon, Oct 1, 2012 at 1:44 PM, Dmitri Gribenko <gribozavr at gmail.com> wrote:

> On Mon, Oct 1, 2012 at 2:34 PM, Alexander Kornienko <alexfh at google.com>
> wrote:
> > On Sat, Sep 29, 2012 at 1:40 PM, Dmitri Gribenko <gribozavr at gmail.com>
> >> +  /// return the last one defined.
> >> +  StringRef getLastMacroWithSpelling(SourceLocation Loc,
> >> +                                     ArrayRef<TokenValue> Tokens)
> const;
> >
> > Isn't it more convenient to have a default spelling as a parameter of
> this
> > method so that callers don't have to check if the result is empty?
>
> It seemed to me to violate the principle of single responsibility.
>

I think the responsibility of this method (may require name change) can be
"give me the most proper spelling for this snippet at this code location".
So providing default value seems to be a good thing to me.

>>
> >> +          SmallString<64> TextToInsert(AnnotationSpelling);
> >> +          TextToInsert += "; ";
> >
> > Over-optimization? ;)
>
> Just the usual thing to use SmallString to avoid dynamic allocations.
>

I know what it does, but I'm not sure that this makes sense in this
particular code location. It's not going to be executed frequently, and
this particular optimization is not going to speed up this code much. I
would prefer to leave it as is, but it's not a big deal.

-- 
Regards,
Alex
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121001/823c7c08/attachment.html>