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

[David Blaikie]

On Mon, Sep 10, 2012 at 8:40 AM, Alexander Kornienko <alexfh at google.com> wrote:
> I could definitely try to find a number of other solutions, but
> unfortunately it would be not fast, as I don't have a configuration that
> allows to reproduce this issue, and I would have to ask the original
> reporter to verify each version.

Ah, I see. I didn't realize you didn't have a local repro. (maybe we
could get some older buildbots up at some point - or at least decide
what we support & what we don't support, version-wise)

> And, after all, is this solution that bad?

I'd say most of us just object on principle - it's not the "right thing".

> What problems can it lead to?

ODR violations (as you say, if the name is sufficiently unique, that's
unlikely). The only other thing I can think of is possible minor build
perf (the function, since it's exported (assuming it's not inlined at
all the call sites anyway & then dropped from the object file
entirely), would end up in some linker data structure to unique any
duplicates that will never arise - not knowing much about the linker I
don't know whether that would increase the size of some common data
structure of all weak symbols & in any way effect the performance)

No worries,

- David

>
>
> On Mon, Sep 10, 2012 at 5:23 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Mon, Sep 10, 2012 at 7:54 AM, Alexander Kornienko <alexfh at google.com>
>> wrote:
>> > Author: alexfh
>> > Date: Mon Sep 10 09:54:38 2012
>> > New Revision: 163513
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=163513&view=rev
>> > Log:
>> > Workaround for MacOSX build failure with gcc <= 4.4
>> >
>> > Summary:
>> > A better solution to http://llvm.org/bugs/show_bug.cgi?id=13777
>> > Named namespace + more unique name to make ODR violations unlikely.
>> >
>> > Reviewers: chandlerc, doug.gregor, klimek
>> >
>> > Reviewed By: doug.gregor
>> >
>> > CC: cfe-commits
>> >
>> > Differential Revision: http://llvm-reviews.chandlerc.com/D38
>> >
>> > 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=163513&r1=163512&r2=163513&view=diff
>> >
>> > ==============================================================================
>> > --- cfe/trunk/tools/clang-check/ClangCheck.cpp (original)
>> > +++ cfe/trunk/tools/clang-check/ClangCheck.cpp Mon Sep 10 09:54:38 2012
>> > @@ -58,10 +58,10 @@
>> >      "ast-dump-filter",
>> >
>> > cl::desc(Options->getOptionHelpText(options::OPT_ast_dump_filter)));
>> >
>> > -// Anonymous namespace here causes problems with gcc <= 4.4 on MacOS:
>> > -// http://llvm.org/bugs/show_bug.cgi?id=13777
>> > -// namespace {
>> > -class ActionFactory {
>> > +// Anonymous namespace here causes problems with gcc <= 4.4 on MacOS
>> > 10.6.
>> > +// "Non-global symbol: ... can't be a weak_definition"
>>
>> Judging by that diagnostic - is it possible that making the
>> newASTConsumer member function non-inline would have been an
>> alternative fix to the code?
>>
>> > +namespace clang_check {
>> > +class ClangCheckActionFactory {
>> >  public:
>> >    clang::ASTConsumer *newASTConsumer() {
>> >      if (ASTList)
>> > @@ -73,10 +73,10 @@
>> >      return new clang::ASTConsumer();
>> >    }
>> >  };
>> > -// }
>> > +}
>> >
>> >  int main(int argc, const char **argv) {
>> > -  ActionFactory Factory;
>> > +  clang_check::ClangCheckActionFactory Factory;
>> >    CommonOptionsParser OptionsParser(argc, argv);
>> >    ClangTool Tool(OptionsParser.GetCompilations(),
>> >                   OptionsParser.GetSourcePathList());
>> >
>> >
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
>
>
> --
> Alexander Kornienko | Software Engineer | alexfh at google.com | +49 151 221 77
> 957
> Google Germany GmbH | Dienerstr. 12 | 80331 München
>