[PATCH] D41897: Fixing a crash in Sema.

[Jan Korous via Phabricator via cfe-commits]

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