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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits