[PATCH] D83211: Factor out call to EXTRACTOR in generateCC1CommandLine

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added inline comments.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3938
+  if ((FLAGS)&options::CC1Option) {                                            \
+    const auto &Extracted = EXTRACTOR(this->KEYPATH);                          \
+    if (ALWAYS_EMIT || Extracted != DEFAULT_VALUE)                             \
----------------
Bigcheese wrote:
> jansvoboda11 wrote:
> > dexonsmith wrote:
> > > dexonsmith wrote:
> > > > jansvoboda11 wrote:
> > > > > Bigcheese wrote:
> > > > > > Will this ever have an issue with lifetime? I can see various values for `EXTRACTOR` causing issues here. https://abseil.io/tips/107
> > > > > > 
> > > > > > 
> > > > > > It would be good to at least document somewhere the restrictions on `EXTRACTOR`.
> > > > > Mentioned the reference lifetime extension in a comment near extractor definitions.
> > > > It might be safer to refactor as:
> > > > ```
> > > > // Capture the extracted value as a lambda argument to avoid potential
> > > > // lifetime extension issues.
> > > > [&](const auto &Extracted) {
> > > >   if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))
> > > >     DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);
> > > > }(EXTRACTOR(this->KEYPATH));
> > > > ```
> > > > 
> > > Might be even better to avoid the generic lambda:
> > > ```
> > > // Capture the extracted value as a lambda argument to avoid potential
> > > // lifetime extension issues.
> > > using ExtractedType =
> > >     std::remove_const_t<std::remove_reference_t<
> > >         decltype(EXTRACTOR(this->KEYPATH))>>
> > > [&](const ExtractedType &Extracted) {
> > >   if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))
> > >     DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);
> > > }(EXTRACTOR(this->KEYPATH));
> > > ```
> > > (since generic vs. non-generic could affect compile-time of CompilerInvocation.cpp given how many instances there will be).
> > Thanks for the suggestions @dexonsmith. I'm having trouble writing a test case where the lambda workaround produces a different result than `const auto &` variable.
> > @Bigcheese, could you show a concrete example of an extractor that causes issues so I can test it out?
> I think I was confused about when this can happen. The `const auto &` will never cause a conversion that would prevent lifetime extension. The only place this would break is if the copy constructor of the type is rather broken, which isn't a reasonable case to support.
Now I remember what the issue was, it's if the `EXTRACTOR` itself creates an owning temporary and converts back to a reference type: https://godbolt.org/z/xsvh4f

This is kind of weird to do, the `EXTRACTOR` would need to take an owning type with an implicit conversion from the type of `KEYPATH`, and then return a reference type. I'm not sure how realistic that situation is, but it seems fine to defend against with Duncan's suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83211