[PATCH] D83211: Factor out call to EXTRACTOR in generateCC1CommandLine
jansvoboda11 updated this revision to Diff 307837.
jansvoboda11 added a comment.
Herald added subscribers: llvm-commits, mgorny.
Herald added a project: LLVM.
Implement review feedback
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83211/new/
https://reviews.llvm.org/D83211
Files:
clang/lib/Frontend/CompilerInvocation.cpp
llvm/unittests/Option/CMakeLists.txt
llvm/unittests/Option/LifetimeOpts.td
Index: llvm/unittests/Option/LifetimeOpts.td
===
--- /dev/null
+++ llvm/unittests/Option/LifetimeOpts.td
@@ -0,0 +1,5 @@
+include "llvm/Option/OptParser.td"
+
+def lifetime_test : Flag<["-"], "lifetime-test">,
+ MarshallingInfoFlag<"FlagValue">,
+ ValueExtractor<"extractorRequiringLifetimeExtension">;
Index: llvm/unittests/Option/CMakeLists.txt
===
--- llvm/unittests/Option/CMakeLists.txt
+++ llvm/unittests/Option/CMakeLists.txt
@@ -4,8 +4,11 @@
)
set(LLVM_TARGET_DEFINITIONS Opts.td)
-
tablegen(LLVM Opts.inc -gen-opt-parser-defs)
+
+set(LLVM_TARGET_DEFINITIONS LifetimeOpts.td)
+tablegen(LLVM LifetimeOpts.inc -gen-opt-parser-defs)
+
add_public_tablegen_target(OptsTestTableGen)
add_llvm_unittest(OptionTests
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -263,6 +263,9 @@
return KeyPath | Value;
}
+// The value returned by an extractor is stored as a constant reference.
+// Watch out for missed reference lifetime extension.
+
template static T extractForwardValue(T KeyPath) {
return KeyPath;
}
@@ -4015,13 +4018,17 @@
void CompilerInvocation::generateCC1CommandLine(
SmallVectorImpl &Args, StringAllocator SA) const {
+ // Capture the extracted value as a lambda argument to avoid potential issues
+ // with lifetime extension of the reference.
#define OPTION_WITH_MARSHALLING(
\
PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,
\
HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,
\
NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX)
\
- if (((FLAGS) & options::CC1Option) &&
\
- (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != (DEFAULT_VALUE))) {
\
-DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH));
\
+ if ((FLAGS)&options::CC1Option) {
\
+[&](const auto &Extracted) {
\
+ if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))
\
+DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);
\
+}(EXTRACTOR(this->KEYPATH));
\
}
#define OPTION_WITH_MARSHALLING_BOOLEAN(
\
@@ -4029,10 +4036,10 @@
HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,
\
NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX, NEG_ID,
\
NEG_SPELLING)
\
- if (((FLAGS)&options::CC1Option) &&
\
- (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE)) {
\
-DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX,
\
- EXTRACTOR(this->KEYPATH));
\
+ if ((FLAGS)&options::CC1Option) {
\
+bool Extracted = EXTRACTOR(this->KEYPATH);
\
+if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))
\
+ DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX, Extracted);
\
}
#include "clang/Driver/Options.inc"
Index: llvm/unittests/Option/LifetimeOpts.td
===
--- /dev/null
+++ llvm/unittests/Option/LifetimeOpts.td
@@ -0,0 +1,5 @@
+include "llvm/Option/OptParser.td"
+
+def lifetime_test : Flag<["-"], "lifetime-test">,
+ MarshallingInfoFlag<"FlagValue">,
+ ValueExtractor<"extractorRequiringLifetimeExtension">;
Index: llvm/unittests/Option/CMakeLists.txt
===
--- llvm/unittests/Option/CMakeLists.txt
+++ llvm/unittests/Option/CMakeLists.txt
@@ -4,8 +4,11 @@
)
set(LLVM_TARGET_DEFINITIONS Opts.td)
-
tablegen(LLVM Opts.inc -gen-opt-parser-defs)
+
+set(LLVM_TARGET_DEFINITIONS LifetimeOpts.td)
+tablegen(LLVM LifetimeOpts.inc -gen-opt-parser-defs)
+
add_public_tablegen_target(OptsTestTableGen)
add_llvm_unittest(OptionTests
Index: clang/lib/Frontend/Compil
[PATCH] D83211: Factor out call to EXTRACTOR in generateCC1CommandLine
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 > > 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
[PATCH] D83211: Factor out call to EXTRACTOR in generateCC1CommandLine
Bigcheese accepted this revision.
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)
\
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 > 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.
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
[PATCH] D83211: Factor out call to EXTRACTOR in generateCC1CommandLine
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 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
[PATCH] D83211: Factor out call to EXTRACTOR in generateCC1CommandLine
dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed. Marking as "requested changes" to get this off my queue, but if my lambda suggestion doesn't work well for some reason (maybe it significantly increases compile-time), I'm open to sticking to the documentation @Bigcheese suggested. 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
[PATCH] D83211: Factor out call to EXTRACTOR in generateCC1CommandLine
dexonsmith 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)
\
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_tKEYPATH))>>
[&](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).
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
[PATCH] D83211: Factor out call to EXTRACTOR in generateCC1CommandLine
dexonsmith 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))
\
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.
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3938
+ if ((FLAGS)&options::CC1Option) {
\
+const auto &Extracted = EXTRACTOR(this->KEYPATH);
\
+if (ALWAYS_EMIT || Extracted != DEFAULT_VALUE)
\
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));
```
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
[PATCH] D83211: Factor out call to EXTRACTOR in generateCC1CommandLine
jansvoboda11 updated this revision to Diff 306035.
jansvoboda11 added a comment.
Rebase
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83211/new/
https://reviews.llvm.org/D83211
Files:
clang/lib/Frontend/CompilerInvocation.cpp
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -263,6 +263,9 @@
return KeyPath | Value;
}
+// The value returned by an extractor is stored as a constant reference.
+// Watch out for missed reference lifetime extension.
+
template static T extractForwardValue(T KeyPath) {
return KeyPath;
}
@@ -4044,9 +4047,10 @@
PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,
\
HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,
\
NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX)
\
- if (((FLAGS) & options::CC1Option) &&
\
- (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != (DEFAULT_VALUE))) {
\
-DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH));
\
+ if ((FLAGS)&options::CC1Option) {
\
+const auto &Extracted = EXTRACTOR(this->KEYPATH);
\
+if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))
\
+ DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);
\
}
#define OPTION_WITH_MARSHALLING_BOOLEAN(
\
@@ -4054,10 +4058,10 @@
HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,
\
NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX, NEG_ID,
\
NEG_SPELLING)
\
- if (((FLAGS)&options::CC1Option) &&
\
- (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE)) {
\
-DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX,
\
- EXTRACTOR(this->KEYPATH));
\
+ if ((FLAGS)&options::CC1Option) {
\
+const auto &Extracted = EXTRACTOR(this->KEYPATH);
\
+if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))
\
+ DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX, Extracted);
\
}
#include "clang/Driver/Options.inc"
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -263,6 +263,9 @@
return KeyPath | Value;
}
+// The value returned by an extractor is stored as a constant reference.
+// Watch out for missed reference lifetime extension.
+
template static T extractForwardValue(T KeyPath) {
return KeyPath;
}
@@ -4044,9 +4047,10 @@
PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,\
HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE, \
NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX) \
- if (((FLAGS) & options::CC1Option) &&\
- (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != (DEFAULT_VALUE))) { \
-DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH)); \
+ if ((FLAGS)&options::CC1Option) {\
+const auto &Extracted = EXTRACTOR(this->KEYPATH); \
+if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE)) \
+ DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);\
}
#define OPTION_WITH_MARSHALLING_BOOLEAN( \
@@ -4054,10 +4058,10 @@
HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE, \
NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX, NEG_ID, \
NEG_SPELLING) \
- if (((FLAGS)&options::CC1Option) && \
- (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE)) {\
-DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX,\
- EXTRACTOR(this->KEYPATH));\
+ if ((FLAGS)&options::CC1Option) {\
+const auto &Extracted = EXTRACTOR(this->KEYPATH); \
+if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE)) \
+ DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX, Extracted); \
[PATCH] D83211: Factor out call to EXTRACTOR in generateCC1CommandLine
jansvoboda11 added a comment.
Addressed review feedback. WDYT @Bigcheese?
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:
> 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.
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
[PATCH] D83211: Factor out call to EXTRACTOR in generateCC1CommandLine
jansvoboda11 updated this revision to Diff 305437.
jansvoboda11 added a comment.
Rebase and add documenting possible reference lifetime extension issue
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83211/new/
https://reviews.llvm.org/D83211
Files:
clang/lib/Frontend/CompilerInvocation.cpp
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -236,6 +236,9 @@
return KeyPath | Value;
}
+// The value returned by an extractor is stored as a constant reference.
+// Watch out for missed reference lifetime extension.
+
template static T extractForwardValue(T KeyPath) {
return KeyPath;
}
@@ -4040,9 +4043,10 @@
PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,
\
HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,
\
TYPE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX)
\
- if (((FLAGS) & options::CC1Option) &&
\
- (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != (DEFAULT_VALUE))) {
\
-DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH));
\
+ if ((FLAGS)&options::CC1Option) {
\
+const auto &Extracted = EXTRACTOR(this->KEYPATH);
\
+if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))
\
+ DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);
\
}
#define OPTION_WITH_MARSHALLING_BOOLEAN(
\
@@ -4050,10 +4054,10 @@
HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,
\
TYPE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX, NEG_ID,
\
NEG_SPELLING)
\
- if (((FLAGS)&options::CC1Option) &&
\
- (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE)) {
\
-DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX,
\
- EXTRACTOR(this->KEYPATH));
\
+ if ((FLAGS)&options::CC1Option) {
\
+const auto &Extracted = EXTRACTOR(this->KEYPATH);
\
+if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))
\
+ DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX, Extracted);
\
}
#include "clang/Driver/Options.inc"
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -236,6 +236,9 @@
return KeyPath | Value;
}
+// The value returned by an extractor is stored as a constant reference.
+// Watch out for missed reference lifetime extension.
+
template static T extractForwardValue(T KeyPath) {
return KeyPath;
}
@@ -4040,9 +4043,10 @@
PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,\
HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE, \
TYPE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX)\
- if (((FLAGS) & options::CC1Option) &&\
- (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != (DEFAULT_VALUE))) { \
-DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH)); \
+ if ((FLAGS)&options::CC1Option) {\
+const auto &Extracted = EXTRACTOR(this->KEYPATH); \
+if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE)) \
+ DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);\
}
#define OPTION_WITH_MARSHALLING_BOOLEAN( \
@@ -4050,10 +4054,10 @@
HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE, \
TYPE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX, NEG_ID,\
NEG_SPELLING) \
- if (((FLAGS)&options::CC1Option) && \
- (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE)) {\
-DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX,\
- EXTRACTOR(this->KEYPATH));\
+ if ((FLAGS)&options::CC1Option) {\
+const auto &Extracted = EXTRACTOR(this->KEYPATH); \
+if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE)) \
+ DENORMALIZ
[PATCH] D83211: Factor out call to EXTRACTOR in generateCC1CommandLine
jansvoboda11 commandeered this revision. jansvoboda11 added a reviewer: dang. jansvoboda11 added a comment. Taking over this patch as Daniel is no longer involved. 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
[PATCH] D83211: Factor out call to EXTRACTOR in generateCC1CommandLine
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)
\
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`.
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
[PATCH] D83211: Factor out call to EXTRACTOR in generateCC1CommandLine
dang updated this revision to Diff 276466.
dang added a comment.
Rebase on top of latest changes
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83211/new/
https://reviews.llvm.org/D83211
Files:
clang/lib/Frontend/CompilerInvocation.cpp
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3934,9 +3934,10 @@
PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,
\
HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,
\
TYPE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX)
\
- if (((FLAGS)&options::CC1Option) &&
\
- (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE)) {
\
-DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH));
\
+ if ((FLAGS)&options::CC1Option) {
\
+const auto &Extracted = EXTRACTOR(this->KEYPATH);
\
+if (ALWAYS_EMIT || Extracted != DEFAULT_VALUE)
\
+ DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);
\
}
#define OPTION_WITH_MARSHALLING_BOOLEAN(
\
@@ -3944,10 +3945,10 @@
HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,
\
TYPE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX, NEG_ID,
\
NEG_SPELLING)
\
- if (((FLAGS)&options::CC1Option) &&
\
- (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE)) {
\
-DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX,
\
- EXTRACTOR(this->KEYPATH));
\
+ if ((FLAGS)&options::CC1Option) {
\
+const auto &Extracted = EXTRACTOR(this->KEYPATH);
\
+if (ALWAYS_EMIT || Extracted != DEFAULT_VALUE)
\
+ DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX, Extracted);
\
}
#include "clang/Driver/Options.inc"
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3934,9 +3934,10 @@
PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,\
HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE, \
TYPE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX)\
- if (((FLAGS)&options::CC1Option) && \
- (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE)) {\
-DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH)); \
+ if ((FLAGS)&options::CC1Option) {\
+const auto &Extracted = EXTRACTOR(this->KEYPATH); \
+if (ALWAYS_EMIT || Extracted != DEFAULT_VALUE) \
+ DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);\
}
#define OPTION_WITH_MARSHALLING_BOOLEAN( \
@@ -3944,10 +3945,10 @@
HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE, \
TYPE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX, NEG_ID,\
NEG_SPELLING) \
- if (((FLAGS)&options::CC1Option) && \
- (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE)) {\
-DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX,\
- EXTRACTOR(this->KEYPATH));\
+ if ((FLAGS)&options::CC1Option) {\
+const auto &Extracted = EXTRACTOR(this->KEYPATH); \
+if (ALWAYS_EMIT || Extracted != DEFAULT_VALUE) \
+ DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX, Extracted); \
}
#include "clang/Driver/Options.inc"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D83211: Factor out call to EXTRACTOR in generateCC1CommandLine
dang created this revision.
dang added a reviewer: Bigcheese.
Herald added subscribers: cfe-commits, dexonsmith.
Herald added a project: clang.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D83211
Files:
clang/lib/Frontend/CompilerInvocation.cpp
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3927,9 +3927,10 @@
PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,
\
HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,
\
TYPE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX)
\
- if (((FLAGS)&options::CC1Option) &&
\
- (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE)) {
\
-DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH));
\
+ if ((FLAGS)&options::CC1Option) {
\
+const auto &Extracted = EXTRACTOR(this->KEYPATH);
\
+if (ALWAYS_EMIT || Extracted != DEFAULT_VALUE)
\
+ DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);
\
}
#include "clang/Driver/Options.inc"
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3927,9 +3927,10 @@
PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,\
HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE, \
TYPE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX)\
- if (((FLAGS)&options::CC1Option) && \
- (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE)) {\
-DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH)); \
+ if ((FLAGS)&options::CC1Option) {\
+const auto &Extracted = EXTRACTOR(this->KEYPATH); \
+if (ALWAYS_EMIT || Extracted != DEFAULT_VALUE) \
+ DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);\
}
#include "clang/Driver/Options.inc"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
