[PATCH] D102343: [clang][patch][FPEnv} Initialization of C++ globals not strictfp aware

Tue Jul 20 11:55:53 PDT 2021

mibintc marked an inline comment as done.
mibintc added a comment.

Question for Aaron

================
Comment at: clang/include/clang/AST/Decl.h:1993
                const DeclarationNameInfo &NameInfo, QualType T,
-               TypeSourceInfo *TInfo, StorageClass S, bool isInlineSpecified,
-               ConstexprSpecKind ConstexprKind,
+               TypeSourceInfo *TInfo, StorageClass S, bool UsesFPIntrin,
+               bool isInlineSpecified, ConstexprSpecKind ConstexprKind,
----------------
aaron.ballman wrote:
> I have no idea if others agree, but I have a slight preference for putting `UsesFPIntrin` towards the end of the parameter list (here and in the Create methods) -- this flag seems less important than the constexpr kind or whether inline is specified (and, it helpfully avoids putting two `bool` parameters next to one another).
> 
> If you agree, perhaps it could be moved to before `TrailingRequiresClause`?
I spent a few hours recoding to rearrange the parameter list, but there are downstream types from FunctionDecl which would presumably also need to be rearranged, and those types add more parameters and some of those parameters have default values, and moving the new boolean before TrailingRequiresClause doesn't work, I decided to throw away the mess I made and ask if you could live with it this way. 

================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:939
+  llvm::RoundingMode RM = getLangOpts().getFPRoundingMode();
+  auto fpExceptionBehavior =
+      ToConstrainedExceptMD(getLangOpts().getFPExceptionMode());
----------------
aaron.ballman wrote:
> Please spell out the type rather than use `auto`, also, should probably be `FP` rather than `fp` per the usual naming conventions.
oops i didn't get this one

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102343