On Thu, Mar 24, 2016 at 11:29 AM, Aaron Ballman <aa...@aaronballman.com> wrote: > On Thu, Mar 24, 2016 at 12:23 PM, David Majnemer > <david.majne...@gmail.com> wrote: >> >> >> On Thu, Mar 24, 2016 at 9:09 AM, Aaron Ballman <aa...@aaronballman.com> >> wrote: >>> >>> On Thu, Mar 24, 2016 at 11:49 AM, Reid Kleckner <r...@google.com> wrote: >>> > On Thu, Mar 3, 2016 at 10:40 AM, Aaron Ballman <aa...@aaronballman.com> >>> > wrote: >>> >> >>> >> That was what I meant by "justification". I would say it has to be >>> >> reasonably compelling code (win32 headers, boost, some other major >>> >> library) as that's our usual bar for these sort of bug-for-bug >>> >> compatible things, as I understand it. >>> > >>> > >>> > I'd rather apply this patch now than wait for ffmpeg or someone to try >>> > to >>> > use static_assert and then have to hustle to get this into clang. Many >>> > many >>> > C projects have COMPILE_ASSERT macros that are just a small change away >>> > from >>> > relying on (_S|s)tatic_assert. >>> >>> I don't find "someone might rely on this bug" to be compelling. Put >>> another way, why do we lower the bar for this bug but not others given >>> that no large projects appear to need this behavior? >> >> >> Very few people are using clang-cl with C code. I'm sure it would come up a >> lot more often if it were better tested. > > Fair, but again, if people aren't hitting it, it's not a problem (yet). ;-) > >> >>> >>> >>> >> >>> >> Agreed, we have a way forward if we need it. I mostly just want to >>> >> avoid the burden of supporting that because this is sufficiently weird >>> >> (being a non-conforming keyword). >>> > >>> > >>> > It's not conforming, but it's not that weird to define our own keywords. >>> > The >>> > C++ committee chose the keyword "static_assert" because it was unlikely >>> > to >>> > conflict with existing code. MSVC has made this a keyword in C mode and >>> > the >>> > world hasn't burned. >>> >>> Correct, we have a way around it, I am just not convinced that we >>> should put forth the effort of supporting another compiler's bug >>> without a compelling use-case. >> >> >> A compelling use case is that users who wish to use static_assert in a >> conforming C program can do so. >> Today, it is impossible to do this with clang-cl & MSVC's CRT.
If someone is trying to use clang and they hit this problem, they can add -Dstatic_assert=_Static_assert to their command line -- or they can fix the code to not rely on non-standard features (or, as the MS engineer suggested, compiler bugs). > Thank you for this explanation, it finally triggered enough of my > brain cells to explain the desire for working around this MSVC bug. > > I think the keyword approach is better than interposing on assert.h in > terms of maintenance, but I would like to see use of static_assert as > a compatibility keyword be ill-formed if the user does not include > assert.h. This gives users a way forward, but helps direct them > towards conforming uses of static_assert so that when MSVC does fix it > and we stop enabling the compatibility mode for it, their code > continues to just work. This isn't 100% compatibility with MSVC (since > they don't require you to include assert.h), but I feel it's > reasonable to tell users to include assert.h to get the behavior they > desire (which continues to work with cl or clang-cl). > > Thoughts? I don't think it's a significant maintenance problem to interpose assert.h; we do that for several other libc headers. That seems a lot simpler than trying to track if assert.h was ever included (especially across PCH / modules). I'm unconvinced we need any change here. In my searching, I've only found one case of someone trying to use this MSVC extension in widespread / popular open source code: https://bugzilla.mozilla.org/show_bug.cgi?id=713531 ... and they wrote code that would have worked fine for clang-cl regardless (and ended up not using the MSVC static_assert anyway). _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
