[clang] Patch for 6037

[Richard Smith]

+typedef std::set<QualType, QualTypeOrdering> IndirectBaseSet;

Use llvm::SmallPtrSet rather than std::set here. Other than that, just
some typographical comments:

+  // Even though the iuncomintincoming type is abase, it might not be
+  // a class -- it could be a template parm, for instamce.

Please have another go at writing this comment =)

+  if (RecordType const *rec = Type->getAs<RecordType>()) {
+    const CXXRecordDecl *decl = rec->getAsCXXRecordDecl();

Please capitalize these variable names for consistency with the rest
of the function. You can also replace both of these lines with

  if (auto *Decl = Type->getAsCXXRecordDecl())

+    // iterate over its bases

Per the LLVM coding style, comments should start with a capital letter
and end with a full stop.


On Fri, Jan 9, 2015 at 7:03 AM, Nathan Sidwell <nathan at acm.org> wrote:
> ping?
>
>
> On 12/24/14 11:46, Nathan Sidwell wrote:
>>
>> On 12/16/14 23:26, Richard Smith wrote:
>>>
>>> On Fri, Dec 12, 2014 at 6:53 AM, Nathan Sidwell <nathan at acm.org
>>> <mailto:nathan at acm.org>> wrote:
>>
>>
>> This 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.
>>
>>> 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.)
>>
>>
>> Patch adjusted.  Builds a std::set of indirect bases of the new class --
>> if it
>> has more than 1 direct base.  Then checks if a direct base is in the set
>> before
>> determining the ambiguity of that base.[*]   Note that a direct and
>> indirect
>> base duplicate could exist, but not be ambiguous iff they are both
>> virtual.
>> Rather than track that data in the std::set, we just check the result of
>> determining the base path ambiguity in the later loop.
>>
>>
>>> def warn_inaccessible_base_class : Warning<
>>>    "direct base %0 is inaccessible due to ambiguity:%1">,
>>>    InGroup<DiagGroup<"warning-flag">>;
>>
>>
>> Done.  Added -W[no-]inaccessible-base
>>
>> A bunch of existing tests trigger.  For some of them I added
>> -Wno-inaccessible-base to the test flags, for some I added the expected
>> warning
>> check -- depending on whether I judged the warning appropriate for the
>> test
>> case, or merely coincidental.  A new test
>> tools/clang/test/SemaCXX/accessible-base.cpp is added
>>
>> built and tested on x86-linux.
>>
>> ok?
>>
>> nathan
>>
>> [*] As an aside, I wonder if completing the std:set with the direct bases
>> and
>> keeping it with the RecordDecl would speed up base conversion checks --
>> the base
>> conversion machinery could use it for a quick 'is this even worth figuring
>> out'
>> check.
>
>