[PATCH] D41897: Fixing a crash in Sema.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: SemaCXX/base-class-ambiguity-check.cpp:1
+// RUN: %clang_cc1 -fsyntax-only %s
+
----------------
jkorous-apple wrote:
> 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?
Weird, it seems Phab didn't have your latest comment when I posted mine -- sorry for the confusion!

"This is enough" != "This is the way it should be done" -- by adding `-verify` and `// expected-no-diagnostics`, you wind up testing that this code doesn't produce any diagnostics in addition to not crashing, which is an improvement over just testing there's no crash, as this code is perfectly reasonable and shouldn't produce a diagnostic.


================
Comment at: SemaCXX/base-class-ambiguity-check.cpp:6
+
+  // Up to commit 680b3a8619 (2018-01-10) this produced a crash in Sema.
+  struct Derived : Base, T {};
----------------
The commit hash doesn't do much here (we're still on svn, but the revision information isn't all that helpful), so I'd replace it with "// Test that this code no longer causes a crash in Sema. See PRNNNN/rdar://NNNN" (assuming you have a report somewhere; if not, just leave off the last sentence).


https://reviews.llvm.org/D41897