aaron.ballman marked an inline comment as not done. aaron.ballman added inline comments.
================ Comment at: clang/include/clang/Basic/LangOptions.def:124 BENIGN_LANGOPT(ImplicitInt, 1, 0, "C89 implicit 'int'") +LANGOPT(StrictPrototypes , 1, 0, "require function types to have a prototype") LANGOPT(Digraphs , 1, 0, "digraphs") ---------------- aaron.ballman wrote: > rsmith wrote: > > This makes me think we should have some declarative way of specifying > > dependencies between `LANGOPT`s. It's presumably sufficiently obvious to a > > library user that they shouldn't enable (eg) `CPlusPlus20` unless they > > enable all the previous `CPlusPlusXY` modes and `CPlusPlus`, but I doubt > > it's obvious that enabling `CPlusPlus` requires also enabling > > `StrictPrototypes`. > > > > In fact, after this change, I think a lot of existing library users of > > Clang that invent their own `LangOptions` will silently start getting this > > wrong. That's concerning. Maybe we should consider prototypes to be > > required if either `StrictPrototypes` or `CPlusPlus` is enabled? > > This makes me think we should have some declarative way of specifying > > dependencies between LANGOPTs. > > Tee hee: > https://github.com/llvm/llvm-project/commit/be9371659380388a693ec99624e1f3d02f07047f > > I was caught by the same issues that I ended up fixing in that commit -- I > tried making `StrictPrototypes` dependent and was very surprised when it > doesn't work. The issue was > `Invocation->getLangOpts()->resetNonModularOptions();` in > `compileModuleImpl()` IIRC -- it would clear all of the language options > (including ones like digraphs and wchar support). > > > In fact, after this change, I think a lot of existing library users of > > Clang that invent their own LangOptions will silently start getting this > > wrong. That's concerning. Maybe we should consider prototypes to be > > required if either StrictPrototypes or CPlusPlus is enabled? > > @cor3ntin also shared this same concern -- my goal with this language option > was: > > * Make it easy to reenable functions without prototypes in C2x mode if we > find some major breakage in the wild (hopefully we don't) > * Make it easy to add `-fstrict-prototypes` as a language flag to opt *into* > strict prototypes (there would be no option to opt *out* of strict > prototypes). > * Stop repeating `CPlusPlus || C2x` in a bunch of places and give it a named > option. > > So I'm not keen on adding `StrictPrototypes || CPlusPlus` everywhere -- C++ > requires strict prototypes. However, the fears are still valid -- what if I > found someplace nice to add an assert that `StrictPrototypes` cannot be false > when `CPlusPlus` is true? >> This makes me think we should have some declarative way of specifying >> dependencies between LANGOPTs. > Tee hee: > https://github.com/llvm/llvm-project/commit/be9371659380388a693ec99624e1f3d02f07047f > > I was caught by the same issues that I ended up fixing in that commit -- I > tried making StrictPrototypes dependent and was very surprised when it > doesn't work. The issue was > Invocation->getLangOpts()->resetNonModularOptions(); in compileModuleImpl() > IIRC -- it would clear all of the language options (including ones like > digraphs and wchar support). There's actually a secondary issue, which is that users can set language options directly -- there's not a setter function for them. This means that places where we create a `LangOptions` object and manually set it up (like our unit tests) have no way to automatically set `StrictPrototypes` when setting other language options. However, this speaks to the importance of having an assert to ensure that by the time the compiler instance is invoked, we have language options that are correct (on the assumption that later stages will not be modifying the language options particularly often). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123955/new/ https://reviews.llvm.org/D123955 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
