[PATCH] D79945: [Sema] Comparison of pointers to complete and incomplete types

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6451
+  "%1 is %select{|in}3complete">,
+  InGroup<C99Compat>;
 def ext_typecheck_ordered_comparison_of_function_pointers : ExtWarn<
----------------
pestctrl wrote:
> efriedma wrote:
> > `InGroup<C11>`
> Sorry, I'm not sure I understand. Isn't this a C99 warning? Why is it being put in the C11 group?
Because `C11` really means `C11Extensions`, and this is a C11 extension (ie, it's code that's valid in C11 but not valid in C99):
```
// A warning group for warnings about using C11 features as extensions.
def C11 : DiagGroup<"c11-extensions">;
```


================
Comment at: clang/lib/Sema/SemaExpr.cpp:11571
+          Diag(Loc,
+               getLangOpts().C11
+                   ? diag::ext_typecheck_compare_complete_incomplete_pointers
----------------
pestctrl wrote:
> rsmith wrote:
> > efriedma wrote:
> > > pestctrl wrote:
> > > > efriedma wrote:
> > > > > rsmith wrote:
> > > > > > pestctrl wrote:
> > > > > > > efriedma wrote:
> > > > > > > > I think this condition is backwards?  Should be `!getLangOpts().C11`.  You want the warning with `-std=c99 -pedantic`, you don't want the warning with `std=c11 -pedantic`.
> > > > > > > I don't think it's backwards. If getLangOpts().C11, then it is an extension. Otherwise, it is the warning. I can switch the conditions if it is confusing though.
> > > > > > "Extension" means "this is invalid code that we're accepting anyway" -- that's what this is in C99. In C11, I think we shouldn't be diagnosing at all.
> > > > > > 
> > > > > > Has anyone checked whether WG14 removed this restriction in C11 as a DR resolution? If so, we shouldn't be diagnosing it at all, in any language mode.
> > > > > I tracked down the proposal for the change; it's http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1439.pdf .  Beyond the reference to http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_314.htm , I can't find any relevant defect report.
> > > > I have updated the diff to diagnose only for C99. Does the existence of the proposal mean we shouldn't be diagnosing in any language mode?
> > > > 
> > > > Also, how did you track down the proposal that quickly? Even after skimming through it, I still can't find it through searching. 
> > > > Also, how did you track down the proposal that quickly? 
> > > 
> > > It wasn't really that quick, but the thing that eventually worked was that I googled for `site:open-std.org "At various points within a translation unit"`.
> > I tried to get some clarity from WG14 as to whether they intended N1439 to be interpreted as applying retroactively, but it seems like their stance is that they do not do maintenance work on past standards, and have no mechanism for identifying whether papers should be encouraged for retroactive application or only for implementations intending to conform to later standards.
> > 
> > In the absence of guidance either way from WG14, I think our best bet is to follow GCC and the literal standards text as this patch does.
> GCC has left this warning on by default in any language mode, FWIW. Should we still restrict this warning to only C99 mode? 
Yes, I think so. I assume the GCC folks haven't noticed that this rule was relaxed in C11. I've filed a bug against GCC for this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95630


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79945/new/

https://reviews.llvm.org/D79945