jkorous-apple added inline comments.
================
Comment at: SemaCXX/base-class-ambiguity-check.cpp:1
+// RUN: %clang_cc1 -fsyntax-only %s
+
----------------
aaron.ballman wrote:
> 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.
No problem, I thought that we are probably not on the same page.
Fair enough. I can do that.
================
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 {};
----------------
aaron.ballman wrote:
> 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).
Oh, right. Thanks.
https://reviews.llvm.org/D41897
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
