[PATCH] D125723: [MSVC] Add support for MSVC pragma optimize

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Parse/ParsePragma.cpp:3662
+  }
+  ExprResult StringResult = Parser::ParseStringLiteralExpression();
+  if (StringResult.isInvalid())
----------------



================
Comment at: clang/lib/Parse/ParsePragma.cpp:3671
   }
   // We could syntax check the string but it's probably not worth the effort.
 
----------------
I think we should be syntax checking the string. Microsoft doesn't, but... what's the benefit to silently doing nothing?


================
Comment at: clang/lib/Sema/SemaAttr.cpp:1099
 
+void Sema::ActOnPragmaMSOptimize(SourceLocation Loc, bool On,
+                                 StringRef OptimizationList) {
----------------
I got tripped up below by seeing `[On]` and thinking it only ever looped over the "enabled" items in the list.


================
Comment at: clang/lib/Sema/SemaAttr.cpp:1108
+    MSOptimizeOperations.clear();
+    if (!On) {
+      MSOptimizeOperations.push_back(&Sema::AddOptnoneAttributeIfNoConflicts);
----------------
It looks like we're not handling this bit of the MS documentation: 
```
Using the optimize pragma with the empty string ("") is a special form of the directive:

...

When you use the on parameter, it resets the optimizations to the ones that you specified using the /O compiler option.
```


================
Comment at: clang/lib/Sema/SemaAttr.cpp:1224
+  FD->dropAttr<OptimizeNoneAttr>();
+  FD->addAttr(NeverOptimizeNoneAttr::CreateImplicit(Context));
+}
----------------
Can you explain why this is needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125723