jansvoboda11 added inline comments.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4062
+ if ((FLAGS)&options::CC1Option) {
\
+ const auto &Extracted = EXTRACTOR(this->KEYPATH);
\
+ if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))
\
----------------
dexonsmith wrote:
> IIUC, then we can do this more simply as:
> ```
> bool Extracted = EXTRACTOR(this->KEYPATH);
> ```
> That might clarify why we don't have lifetime extension concerns here.
Yes, we could simplify this for booleans.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3938
+ if ((FLAGS)&options::CC1Option) {
\
+ const auto &Extracted = EXTRACTOR(this->KEYPATH);
\
+ if (ALWAYS_EMIT || Extracted != DEFAULT_VALUE)
\
----------------
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?
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
