aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed.
Thank you for working on this, and for your patience -- this review fell off my radar for a bit, sorry about that! I think there's an issue here to be solved, but I'm not certain this is the correct way to solve it. Giving two or more diagnostics for the same underlying issue is generally something we try not to do, but happens as a result of your changes. However, this code should diagnose all four times, not just twice as it currently does, so there is a bug to fix: https://godbolt.org/z/avPj5q7hc ================ Comment at: clang/lib/Sema/SemaChecking.cpp:13555-13557 + DiagnoseImpCast(S, E, T, CC, DiagID); + if (!isa<EnumType>(Target) || !isa<EnumType>(Source)) + return; ---------------- I don't think this change is correct -- we *want* to silence the conversion warnings in this case. GCC behaves that way as well: https://godbolt.org/z/oona5Mr5T ================ Comment at: clang/test/Sema/enum-enum-conversion.c:10-14 +#ifdef ENUM_CONV + return E; // expected-warning {{implicit conversion from enumeration type 'enum PE1' to different enumeration type 'enum PE2'}} +#else + return E; // expected-warning {{implicit conversion from enumeration type 'enum PE1' to different enumeration type 'enum PE2'}} +#endif ---------------- Testing tip: instead of using the preprocessor and duplicating so much, you can use a feature of `-verify` where you give it a prefix. e.g., ``` // RUN: ... -verify=stuff ... // RUN: ... -verify=other ... void func(void) { thing(); // stuff-warning {{"this is a warning about stuff}} \ other-error {{"this is an error, it's different than stuff}} } ``` Using this sort of style makes the tests much easier to read. ================ Comment at: clang/test/Sema/enum-enum-conversion.c:40 +} \ No newline at end of file ---------------- Please add a newline to the end of the file. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123009/new/ https://reviews.llvm.org/D123009 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
