[cfe-commits] r160281 - /cfe/trunk/include/clang/AST/RecursiveASTVisitor.h

[Aaron Ballman]

On Tue, Jul 17, 2012 at 12:26 AM, Douglas Gregor <dgregor at apple.com> wrote:
>
> On Jul 16, 2012, at 8:45 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
>
>> Author: aaronballman
>> Date: Mon Jul 16 10:45:33 2012
>> New Revision: 160281
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=160281&view=rev
>> Log:
>> Fixing an MSVC warning -- the compiler did not like the cast added to work around a g++ bug (it would claim to possibly emit incorrect code).
>>
>> Modified:
>>    cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
>>
>> Modified: cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/RecursiveASTVisitor.h?rev=160281&r1=160280&r2=160281&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/AST/RecursiveASTVisitor.h (original)
>> +++ cfe/trunk/include/clang/AST/RecursiveASTVisitor.h Mon Jul 16 10:45:33 2012
>> @@ -464,12 +464,19 @@
>> bool RecursiveASTVisitor<Derived>::dataTraverseNode(Stmt *S,
>>                                                     bool &EnqueueChildren) {
>>
>> +// The cast for DISPATCH_WALK is needed for older versions of g++, but causes
>> +// problems for MSVC.  So we'll skip the cast entirely for MSVC.
>> +#if defined(_MSC_VER)
>> +  #define GCC_CAST(CLASS)
>> +#else
>> +  #define GCC_CAST(CLASS) (bool (RecursiveASTVisitor::*)(CLASS*))
>> +#endif
>> +
>>   // Dispatch to the corresponding WalkUpFrom* function only if the derived
>>   // class didn't override Traverse* (and thus the traversal is trivial).
>> -  // The cast here is necessary to work around a bug in old versions of g++.
>> #define DISPATCH_WALK(NAME, CLASS, VAR) \
>>   if (&RecursiveASTVisitor::Traverse##NAME == \
>> -      (bool (RecursiveASTVisitor::*)(CLASS*))&Derived::Traverse##NAME) \
>> +      GCC_CAST(CLASS)&Derived::Traverse##NAME) \
>>     return getDerived().WalkUpFrom##NAME(static_cast<CLASS*>(VAR)); \
>>   EnqueueChildren = false; \
>>   return getDerived().Traverse##NAME(static_cast<CLASS*>(VAR));
>> @@ -509,6 +516,7 @@
>>   }
>
> This GCC_CAST thing is pretty hideous. Is there no non-macro, well-formed way to appease both compilers? Like, for example, creating a local variable of type bool (recursiveASTVisitor::*)(CLASS*)?

I agree that it's not the prettiest solution, but I've not found a way
that I'm comfortable with to appease both compilers.  I can test MSVC
easily enough, but I have no idea what older version of g++ was
exhibiting the behavior requiring the cast in the first place, or how
the bug manifested itself there.  So I'm worried about regressing the
issue unintentionally.  At least this way, the change has minimal
impact while solving the problem (even if it's pretty ugly).

I'm open to suggestions on how to verify other changes though!

~Aaron