[PATCH] D157046: [clang] Abstract away string allocation in command line generation

2023-08-03 Thread Jan Svoboda via Phabricator via cfe-commits
jansvoboda11 added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4323
+GenerateArg(Consumer, OPT_darwin_target_variant_sdk_version_EQ,
+Opts.DarwinTargetVariantSDKVersion.getAsString());
 }

benlangmuir wrote:
> Maybe not worth micro optimizing, but I noticed these two are allocating 
> strings unnecessarily if we had an overload for things that can print to a 
> raw_ostream.
Interesting, there are a couple of other instances where this might help. I 
probably won't be spending time on this right now, but good to be aware.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157046

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D157046: [clang] Abstract away string allocation in command line generation

2023-08-03 Thread Jan Svoboda via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rG83452650490e: [clang] Abstract away string allocation in 
command line generation (authored by jansvoboda11).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157046

Files:
  clang/include/clang/Frontend/CompilerInvocation.h
  clang/lib/Frontend/CompilerInvocation.cpp

Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -165,6 +165,8 @@
 // Normalizers
 //===--===//
 
+using ArgumentConsumer = CompilerInvocation::ArgumentConsumer;
+
 #define SIMPLE_ENUM_VALUE_TABLE
 #include "clang/Driver/Options.inc"
 #undef SIMPLE_ENUM_VALUE_TABLE
@@ -191,13 +193,10 @@
 /// denormalizeSimpleFlags never looks at it. Avoid bloating compile-time with
 /// unnecessary template instantiations and just ignore it with a variadic
 /// argument.
-static void denormalizeSimpleFlag(SmallVectorImpl ,
-  const Twine ,
-  CompilerInvocation::StringAllocator,
-  Option::OptionClass, unsigned, /*T*/...) {
-  // Spelling is already allocated or a static string, no need to call SA.
-  assert(*Spelling.getSingleStringRef().end() == '\0');
-  Args.push_back(Spelling.getSingleStringRef().data());
+static void denormalizeSimpleFlag(ArgumentConsumer Consumer,
+  const Twine , Option::OptionClass,
+  unsigned, /*T*/...) {
+  Consumer(Spelling);
 }
 
 template  static constexpr bool is_uint64_t_convertible() {
@@ -234,34 +233,27 @@
 }
 
 static auto makeBooleanOptionDenormalizer(bool Value) {
-  return [Value](SmallVectorImpl , const Twine ,
- CompilerInvocation::StringAllocator, Option::OptionClass,
- unsigned, bool KeyPath) {
-if (KeyPath == Value) {
-  // Spelling is already allocated or a static string, no need to call SA.
-  assert(*Spelling.getSingleStringRef().end() == '\0');
-  Args.push_back(Spelling.getSingleStringRef().data());
-}
+  return [Value](ArgumentConsumer Consumer, const Twine ,
+ Option::OptionClass, unsigned, bool KeyPath) {
+if (KeyPath == Value)
+  Consumer(Spelling);
   };
 }
 
-static void denormalizeStringImpl(SmallVectorImpl ,
+static void denormalizeStringImpl(ArgumentConsumer Consumer,
   const Twine ,
-  CompilerInvocation::StringAllocator SA,
   Option::OptionClass OptClass, unsigned,
   const Twine ) {
   switch (OptClass) {
   case Option::SeparateClass:
   case Option::JoinedOrSeparateClass:
   case Option::JoinedAndSeparateClass:
-// Spelling is already allocated or a static string, no need to call SA.
-assert(*Spelling.getSingleStringRef().end() == '\0');
-Args.push_back(Spelling.getSingleStringRef().data());
-Args.push_back(SA(Value));
+Consumer(Spelling);
+Consumer(Value);
 break;
   case Option::JoinedClass:
   case Option::CommaJoinedClass:
-Args.push_back(SA(Twine(Spelling) + Value));
+Consumer(Spelling + Value);
 break;
   default:
 llvm_unreachable("Cannot denormalize an option with option class "
@@ -270,11 +262,10 @@
 }
 
 template 
-static void
-denormalizeString(SmallVectorImpl , const Twine ,
-  CompilerInvocation::StringAllocator SA,
-  Option::OptionClass OptClass, unsigned TableIndex, T Value) {
-  denormalizeStringImpl(Args, Spelling, SA, OptClass, TableIndex, Twine(Value));
+static void denormalizeString(ArgumentConsumer Consumer, const Twine ,
+  Option::OptionClass OptClass, unsigned TableIndex,
+  T Value) {
+  denormalizeStringImpl(Consumer, Spelling, OptClass, TableIndex, Twine(Value));
 }
 
 static std::optional
@@ -315,15 +306,14 @@
   return std::nullopt;
 }
 
-static void denormalizeSimpleEnumImpl(SmallVectorImpl ,
+static void denormalizeSimpleEnumImpl(ArgumentConsumer Consumer,
   const Twine ,
-  CompilerInvocation::StringAllocator SA,
   Option::OptionClass OptClass,
   unsigned TableIndex, unsigned Value) {
   assert(TableIndex < SimpleEnumValueTablesSize);
   const SimpleEnumValueTable  = SimpleEnumValueTables[TableIndex];
   if (auto MaybeEnumVal = findValueTableByValue(Table, Value)) {
-denormalizeString(Args, Spelling, SA, OptClass, TableIndex,
+denormalizeString(Consumer, Spelling, OptClass, TableIndex,
   

[PATCH] D157046: [clang] Abstract away string allocation in command line generation

2023-08-03 Thread Ben Langmuir via Phabricator via cfe-commits
benlangmuir accepted this revision.
benlangmuir added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4323
+GenerateArg(Consumer, OPT_darwin_target_variant_sdk_version_EQ,
+Opts.DarwinTargetVariantSDKVersion.getAsString());
 }

Maybe not worth micro optimizing, but I noticed these two are allocating 
strings unnecessarily if we had an overload for things that can print to a 
raw_ostream.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157046

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits