[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

smeenai added a comment. Similar to the case @mstorsjo mentioned, this also causes the following code to fail now: struct S { unsigned size; }; template struct U { S s; unsigned size() { return s.size(); } }; This is obviously invalid, and I personally prefer

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

mstorsjo added a comment. In D69950#1771515 , @rnk wrote: > I fixed this particular code upstream: > https://github.com/KhronosGroup/glslang/pull/2010 > I am not enough an expert to be sure, but I suspect this is in the area of > "invalid, no

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

rnk added a comment. In D69950#1770138 , @mstorsjo wrote: > This (when reapplied in > https://reviews.llvm.org/rG878a24ee244a24c39d1c57e9af2) broke compilation of > code that earlier built fine. A reduced example: > > namespace glslang { > class

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

erichkeane added a comment. In D69950#1771462 , @eandrews wrote: > In D69950#1771340 , @mstorsjo wrote: > > > In D69950#1771133 , @eandrews > > wrote: > > > > > In

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

eandrews added a comment. In D69950#1771340 , @mstorsjo wrote: > In D69950#1771133 , @eandrews wrote: > > > In D69950#1770138 , @mstorsjo > > wrote: > > > > > This (when

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

mstorsjo added a comment. In D69950#1771133 , @eandrews wrote: > In D69950#1770138 , @mstorsjo wrote: > > > This (when reapplied in > > https://reviews.llvm.org/rG878a24ee244a24c39d1c57e9af2) broke compilation >

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

eandrews added a comment. In D69950#1770138 , @mstorsjo wrote: > This (when reapplied in > https://reviews.llvm.org/rG878a24ee244a24c39d1c57e9af2) broke compilation of > code that earlier built fine. A reduced example: > > namespace glslang { >

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

mstorsjo added a comment. This (when reapplied in https://reviews.llvm.org/rG878a24ee244a24c39d1c57e9af2) broke compilation of code that earlier built fine. A reduced example: namespace glslang { class TPoolAllocator { void operator=(TPoolAllocator); }; template class a {

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

eandrews added a comment. In D69950#1752044 , @gribozavr2 wrote: > In D69950#1751894 , @eandrews wrote: > > > In D69950#1751859 , @gribozavr2 > > wrote: > > > > > > hence

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

gribozavr2 added a comment. In D69950#1751894 , @eandrews wrote: > In D69950#1751859 , @gribozavr2 > wrote: > > > > hence not throwing the warning on any platform? > > > > The way I read the buildbot breakage, an

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

eandrews added a comment. In D69950#1751859 , @gribozavr2 wrote: > > hence not throwing the warning on any platform? > > The way I read the buildbot breakage, an existing ClangTidy test passed > before and after this change, but broke on Windows. The

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

gribozavr2 added a comment. > hence not throwing the warning on any platform? I'm not sure I understand? The way I read the buildbot breakage, an existing ClangTidy test passed before and after this change, but broke on Windows. The breakage was that the warnings stopped being produced.

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

eandrews added a comment. The patch changes type dependency of field 'std::string s' and sets it based on field type (i.e. not type-dependent). I do not know how clang-tidy works internally, but I assume checker used to skip the field earlier since it was set as dependent (based on containing

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

gribozavr2 added a comment. A number of ClangTidy tests have `-fno-delayed-template-parsing` in their RUN lines. While not a great solution (it only fixes the test, but does not make the warnings show for users on Windows), there's only so much a checker can do to counteract that. Before

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

eandrews added a comment. Thank you for letting me know. The patch has been reverted. The fail is a result of different diagnostics thrown on Windows and Linux. It looks like clang passes -fdelayed-template-parsing by default on Windows, which I assume is the reason why the diagnostic is not

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

gribozavr2 added a comment. > Should I go ahead and revert my patch while I investigate the issue on > Windows or should I just push a new patch deleting the 'CHECK-FIXES' while I > look into the issue? Yes. As a general rule, if you landed a patch and it broke a bot that was previously

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

eandrews added a comment. @rnk @gribozavr2 clang-tidy test "bugprone-string-integer-assignment.cpp" caused a bot fail on windows. From the logs it looks like the 'CHECK-FIXES' I added in that test is failing because the fix isn't applied. I am not sure why. I don't have a windows build set up

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

This revision was automatically updated to reflect the committed changes. Closed by commit rG759948467ea3: Reapply Fix crash on switch conditions of non-integer types in templates (authored by mibintc). Herald added a project: clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

eandrews added a subscriber: mibintc. eandrews added a comment. Thank you for taking a look Reid and Dmitri. There were no fails with check-all. My commit rights have not been transferred from SVN to Github yet, so Melanie (@mibintc) has kindly agreed to commit this patch for me. CHANGES

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

gribozavr2 accepted this revision. gribozavr2 added a comment. LGTM for ClangTidy, assuming you ran `ninja check-all`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69950/new/ https://reviews.llvm.org/D69950 ___ cfe-commits mailing list

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. I don't have experience with clang-tidy tests, but I think these two small, test-only changes are small enough that you can go ahead and commit them without review from a clang-tidy owner. lgtm

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

eandrews created this revision. eandrews added reviewers: rnk, gribozavr, gribozavr2, Szelethus, erichkeane, riccibruno. This patch reapplies D61027 . When D61027 was previously committed (76945821b9cad3), buildbots failed due