[PATCH] Improved llvm-namespace-comment check.

[Manuel Klimek]

On Mon, May 19, 2014 at 7:04 PM, Manuel Klimek <klimek at google.com> wrote:

> Oh, I thought NextToken is the comment token... Now I'm thoroughly
> confused.
>

Perhaps:
// We only add a newline if the next token is not a comment token and it's
on the same line as the closing rbrace.

But then perhaps we can write the condition somewhat more simple?


>
>
> On Mon, May 19, 2014 at 6:59 PM, Alexander Kornienko <alexfh at google.com>wrote:
>
>> On Mon, May 19, 2014 at 6:57 PM, Manuel Klimek <klimek at google.com> wrote:
>>
>>> On Mon, May 19, 2014 at 6:55 PM, Alexander Kornienko <alexfh at google.com>wrote:
>>>
>>>> On Mon, May 19, 2014 at 6:54 PM, Alexander Kornienko <alexfh at google.com
>>>> > wrote:
>>>>
>>>>> On Mon, May 19, 2014 at 6:53 PM, Manuel Klimek <klimek at google.com>wrote:
>>>>>
>>>>>> On Mon, May 19, 2014 at 6:29 PM, Manuel Klimek <klimek at google.com>wrote:
>>>>>>
>>>>>>> lg
>>>>>>>
>>>>>>> ================
>>>>>>> Comment at: clang-tidy/llvm/NamespaceCommentCheck.cpp:66
>>>>>>> @@ +65,3 @@
>>>>>>> +  Token Tok;
>>>>>>> +  while (Lexer::getRawToken(Loc, Tok, Sources,
>>>>>>> Result.Context->getLangOpts())) {
>>>>>>> +    Loc = Loc.getLocWithOffset(1);
>>>>>>> ----------------
>>>>>>> Can you add a comment explaining why it's ok to retry on failure
>>>>>>> with an incremented location?
>>>>>>>
>>>>>>> ================
>>>>>>> Comment at: clang-tidy/llvm/NamespaceCommentCheck.cpp:73
>>>>>>> @@ +72,3 @@
>>>>>>> +  bool NextTokenIsOnSameLine = Sources.getSpellingLineNumber(Loc)
>>>>>>> == EndLine;
>>>>>>> +  bool NeedLineBreak = NextTokenIsOnSameLine && Tok.isNot(tok::eof);
>>>>>>> +
>>>>>>> ----------------
>>>>>>> Can you explain why we need this? (I'd have expected the fix-it to
>>>>>>> not change the number of newlines, thus just inserting the comment between
>>>>>>> the } and the newline).
>>>>>>>
>>>>>>
>>>>>> Am I missing where you added a comment for this?
>>>>>>
>>>>>
>>>>> Should be here:
>>>>> http://reviews.llvm.org/D3825?vs=9577&id=9579&whitespace=ignore-all#toc
>>>>>
>>>>>
>>>>
>>>> Ah, sorry, there's no comment for this. I've answered your question on
>>>> the Phab only.
>>>> Should I add a comment?
>>>>
>>>
>>> Yes, please :) Thx!
>>>
>>
>>
>> Looking at this once again, I see that a comment would be tautological:
>>   // We need a line break when the next token is on the same line.
>>    bool NeedLineBreak = NextTokenIsOnSameLine && Tok.isNot(tok::eof);
>>
>> Are you sure it will make anything clearer?
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140519/eaaeb863/attachment.html>