[cfe-commits] r163429 - /cfe/trunk/tools/clang-check/ClangCheck.cpp

[Alexander Kornienko]

Committed as r163513.

On Mon, Sep 10, 2012 at 1:31 PM, Alexander Kornienko <alexfh at google.com>wrote:

> Douglas, Chandler,
>
> Please, take a look at the updated patch here:
> http://llvm-reviews.chandlerc.com/D38
>
> On Mon, Sep 10, 2012 at 12:55 PM, Alexander Kornienko <alexfh at google.com>wrote:
>
>>
>>
>> On Mon, Sep 10, 2012 at 12:07 AM, Douglas Gregor <dgregor at apple.com>wrote:
>>
>>>
>>> Sent from my iPhone
>>> On Sep 8, 2012, at 6:55 PM, Chandler Carruth <chandlerc at google.com>
>>> wrote:
>>>
>>> On Sat, Sep 8, 2012 at 6:10 PM, Douglas Gregor <dgregor at apple.com>wrote:
>>>
>>>> Sent from my iPhone
>>>>
>>>> On Sep 7, 2012, at 6:44 PM, Alexander Kornienko <alexfh at google.com>
>>>> wrote:
>>>>
>>>> > Author: alexfh
>>>> > Date: Fri Sep  7 17:44:34 2012
>>>> > New Revision: 163429
>>>> >
>>>> > URL: http://llvm.org/viewvc/llvm-project?rev=163429&view=rev
>>>> > Log:
>>>> > Fixed http://llvm.org/bugs/show_bug.cgi?id=13777
>>>> >
>>>> > Modified:
>>>> >    cfe/trunk/tools/clang-check/ClangCheck.cpp
>>>> >
>>>> > Modified: cfe/trunk/tools/clang-check/ClangCheck.cpp
>>>> > URL:
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/clang-check/ClangCheck.cpp?rev=163429&r1=163428&r2=163429&view=diff
>>>> >
>>>> ==============================================================================
>>>> > --- cfe/trunk/tools/clang-check/ClangCheck.cpp (original)
>>>> > +++ cfe/trunk/tools/clang-check/ClangCheck.cpp Fri Sep  7 17:44:34
>>>> 2012
>>>> > @@ -58,7 +58,9 @@
>>>> >     "ast-dump-filter",
>>>> >
>>>> cl::desc(Options->getOptionHelpText(options::OPT_ast_dump_filter)));
>>>> >
>>>> > -namespace {
>>>> > +// Anonymous namespace here causes problems with gcc <= 4.4 on MacOS:
>>>> > +// http://llvm.org/bugs/show_bug.cgi?id=13777
>>>> > +// namespace {
>>>>
>>>> How about just putting it in the clang namespace? At the very least,
>>>> please delete the commented-out lines.
>>>>
>>>
>>> Agreed on both fronts. Also, bugs are usually cited just as "PR13777",
>>> and almost never cited in the source code, but rather in the test case
>>> itself.
>>>
>>>
>>> The reasoning behind this is that the code and comments should stand
>>> alone. Nobody should have to hunt through a bug tracker to determine the
>>> intent.
>>>
>> Thanks for the explanation, I didn't know this point.
>>
>>
>>> The test suite, on the other hand, is a mix of feature tests and
>>> regression tests that often grows in response to specific bugs, and the
>>> cross-linking to those bugs is a valuable addition.
>>>
>> This is overall a reasonable statement. As for this specific case, I'm
>> not sure I can come up with a sane test for this (this is not a
>> functionality after all, and the bug is not in the source code, but in an
>> external compiler). That's why I decided to put a link to the bug tracker
>> item in the source file itself. And the second reason is that I wanted to
>> provide more context to those who'll read this code, in addition to the
>> short description immediately in comments.
>>
>>
>>> That said, is this really the necessary solution? There are a lot of
>>> template arguments defined in an anonymous namespace within the codebase
>>> already. How do they work without problems?
>>>
>>>
>>> Yes, this is odd.
>>>
>> I would suggest, that this can be the only place where a class from an
>> unnamed namespace is used as a parameter of a template function, which has
>> a local class. But I certainly don't want to find a complete and correct
>> proof of this statement. It could be rather time consuming, and I'm sure it
>> won't have much value.
>>
>>
>>> I feel like this is an ODR violation waiting to happen unless we put it
>>> in an anonymous namespace the way it should be...
>>>
>>> -Chandler
>>>
>>>
>> --
>> Regards,
>> Alex
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120910/458ccaaf/attachment.html>