jkorous-apple added inline comments.
================
Comment at: Sema/SemaDeclCXX.cpp:2426
+ // Skip all dependent types in templates being used as base specifiers.
+ // Checks below assume that base specifier is a CXXRecord.
+ if (BaseType->isDependentType()) {
----------------
aaron.ballman wrote:
> that base -> that the base
Good catch. Thanks.
================
Comment at: Sema/SemaDeclCXX.cpp:2427-2429
+ if (BaseType->isDependentType()) {
+ continue;
+ }
----------------
aaron.ballman wrote:
> You can elide the braces here.
Of course. Thanks for reminder.
================
Comment at: SemaCXX/base-class-ambiguity-check.cpp:1
+// RUN: %clang_cc1 -fsyntax-only %s
+
----------------
aaron.ballman wrote:
> This run line isn't testing anything. Since you're trying to ensure this
> doesn't crash, I would put `-verify` on the RUN line and `//
> expected-no-diagnostics` on the line below.
I know what you mean. I was worried about that as well so I asked other guys.
Apparently this is enough. If you run the test on master this is what you get:
```
> bin/llvm-lit
> /Users/jankorous/src/oss/llvm/llvm/tools/clang/test/SemaCXX/base-class-ambiguity-check.cpp
llvm-lit: /Users/jankorous/src/oss/llvm/llvm/utils/lit/lit/llvm/config.py:334:
note: using clang: /Users/jankorous/src/oss/llvm/build/bin/clang
llvm-lit: /Users/jankorous/src/oss/llvm/llvm/utils/lit/lit/util.py:379: note:
using SDKROOT:
'/Applications/Edge9E77_AllSDKs_fromNFA/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.13.sdk'
-- Testing: 1 tests, 1 threads --
FAIL: Clang :: SemaCXX/base-class-ambiguity-check.cpp (1 of 1)
Testing Time: 0.31s
********************
Failing Tests (1):
Clang :: SemaCXX/base-class-ambiguity-check.cpp
Unexpected Failures: 1
```
================
Comment at: SemaCXX/base-class-ambiguity-check.cpp:1
+// RUN: %clang_cc1 -fsyntax-only %s
+
----------------
aaron.ballman wrote:
> jkorous-apple wrote:
> > aaron.ballman wrote:
> > > This run line isn't testing anything. Since you're trying to ensure this
> > > doesn't crash, I would put `-verify` on the RUN line and `//
> > > expected-no-diagnostics` on the line below.
> > I know what you mean. I was worried about that as well so I asked other
> > guys. Apparently this is enough. If you run the test on master this is what
> > you get:
> >
> >
> > ```
> > > bin/llvm-lit
> > > /Users/jankorous/src/oss/llvm/llvm/tools/clang/test/SemaCXX/base-class-ambiguity-check.cpp
> > llvm-lit:
> > /Users/jankorous/src/oss/llvm/llvm/utils/lit/lit/llvm/config.py:334: note:
> > using clang: /Users/jankorous/src/oss/llvm/build/bin/clang
> > llvm-lit: /Users/jankorous/src/oss/llvm/llvm/utils/lit/lit/util.py:379:
> > note: using SDKROOT:
> > '/Applications/Edge9E77_AllSDKs_fromNFA/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.13.sdk'
> > -- Testing: 1 tests, 1 threads --
> > FAIL: Clang :: SemaCXX/base-class-ambiguity-check.cpp (1 of 1)
> > Testing Time: 0.31s
> > ********************
> > Failing Tests (1):
> > Clang :: SemaCXX/base-class-ambiguity-check.cpp
> >
> > Unexpected Failures: 1
> >
> > ```
> This comment hasn't been applied yet.
Sorry, what do you mean?
================
Comment at: SemaCXX/base-class-ambiguity-check.cpp:9
+
+ struct Derived : Base, T
+ { };
----------------
aaron.ballman wrote:
> I would add a comment around here explaining that this used to crash.
Good idea.
================
Comment at: SemaCXX/base-class-ambiguity-check.cpp:12
+};
\ No newline at end of file
----------------
aaron.ballman wrote:
> Can you add a newline at the end of the file, and then run the file through
> clang-format to properly format it?
Sure. I forgot to clang-format the test.
https://reviews.llvm.org/D41897
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
