[clang] Patch for 6037

[Richard Smith]

On Fri, Dec 12, 2014 at 6:53 AM, Nathan Sidwell <nathan at acm.org> wrote:

> ping?
>
>
> On 12/05/14 16:57, Nathan Sidwell wrote:
>
>> This (proto) patch addresses bug 6037 http://llvm.org/bugs/show_bug.
>> cgi?id=6037
>> A request for a class definition to warn about inaccessible direct base
>> classes.
>>   I've  added a check in Sema::AttachBaseSpecifiers to iterate over the
>> direct
>> base array looking for ambiguous conversions to the individual bases.  The
>> output is something like:
>>
>> 6037.cc:8:15: warning: direct base 'A' is inaccessible due to ambiguity:
>>      struct C -> struct B -> struct A
>>      struct C -> struct A
>> struct C : B, A {
>>                ^
>> which matches the form of error for an ambiguous base conversion itself.
>>
>
This seems like a good idea for a warning to me.

This patch triggers on a pile of testcases, which I've not yet fixed,
>> because
>> I'd like some feedback on this approach first -- is there perhaps a
>> better way
>> to do this checking?
>>
>
+    CXXBasePaths Paths(/*FindAmbiguities=*/true, /*RecordPaths=*/true,
+                     /*DetectVirtual=*/true);
+    (void)IsDerivedFrom(Context.getTagDeclType(Class), BaseType, Paths);
+    if (Paths.isAmbiguous(Context.getCanonicalType(BaseType)
+   .getUnqualifiedType ()))

This will do a lot of redundant work if there are multiple base classes.
Instead, construct the CXXBasePaths object outside of the loop and
initialize it by calling Class->lookupInBases with a predicate that always
matches, then just call Paths.isAmbiguous within the loop.

That said, you should be able to do this even more efficiently by
collecting a list of all indirect base classes then checking if any direct
base class is on the list. (If any is, you'll still need the code you
already have, in order to describe the ambiguity.)

Also, as this is a warning, I presume there should be some way to disable
>> it.
>> How is that achieved in the clang framework?  (I'm not sure if the now
>> failing
>> tests should be fixed by disabling the warning, for instance).
>
>
Like so:

def warn_inaccessible_base_class : Warning<
  "direct base %0 is inaccessible due to ambiguity:%1">,
  InGroup<DiagGroup<"warning-flag">>;

If there are multiple warnings controlled by the flag (or you want to put
this group inside another group), the DiagGroup should be factored out (see
DiagnosticGroups.td).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141216/c8f87e83/attachment.html>