[PATCH] Improved llvm-namespace-comment check.
Alexander Kornienko
alexfh at google.com
Mon May 19 09:59:50 PDT 2014
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/5b43c8bf/attachment.html>
More information about the cfe-commits
mailing list