Committed as r163513. On Mon, Sep 10, 2012 at 1:31 PM, Alexander Kornienko <[email protected]>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 > <[email protected]>wrote: > >> >> >> On Mon, Sep 10, 2012 at 12:07 AM, Douglas Gregor <[email protected]>wrote: >> >>> >>> Sent from my iPhone >>> On Sep 8, 2012, at 6:55 PM, Chandler Carruth <[email protected]> >>> wrote: >>> >>> On Sat, Sep 8, 2012 at 6:10 PM, Douglas Gregor <[email protected]>wrote: >>> >>>> Sent from my iPhone >>>> >>>> On Sep 7, 2012, at 6:44 PM, Alexander Kornienko <[email protected]> >>>> 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 >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
