erichkeane added inline comments.
================
Comment at: lib/Sema/SemaChecking.cpp:110
+/// are usually useless
+static unsigned AdjustPrecision(unsigned precision) {
+ return (precision * 59 + 195) / 196;
----------------
Hmm.... I don't terribly understand how this function works. Also, comment
above needs to end in a period.
Can you elaborate further as to how this works? Those are 3 pretty suspect
looking magic numbers...
================
Comment at: lib/Sema/SemaChecking.cpp:10862
+ if (Source->isIntegerType() && Target->isFloatingType()) {
+ const llvm::fltSemantics *FloatSem = nullptr;
+ if (Target->isSpecificBuiltinType(BuiltinType::Float)) {
----------------
Since there is only 1, and fltSemantics is really small (2 shorts + 2 uints),
making this a 'by copy' is likely a better solution than a pointer.
================
Comment at: lib/Sema/SemaChecking.cpp:10867
+ FloatSem = &llvm::APFloat::IEEEdouble();
+ }
+
----------------
This else case ends up needing to be handled above, but why not also handle the
other floating types?
================
Comment at: lib/Sema/SemaChecking.cpp:10873
+ if (E->EvaluateAsInt(IntValue, S.Context, Expr::SE_AllowSideEffects)) {
+ if (S.SourceMgr.isInSystemMacro(CC))
+ return;
----------------
It seems like this should be checked way before we convert it to an integer or
do the other float-semantics disambiguation.
================
Comment at: lib/Sema/SemaChecking.cpp:10877
+ if (FloatValue.convertFromAPInt(IntValue,
Source->isSignedIntegerType(),
+ llvm::APFloat::rmTowardZero) !=
+ llvm::APFloat::opOK) {
----------------
Is this the rounding mode that we use? Also, you might need to check this
against some sort of current state for rounding mode. I know that there is an
effort to do FENV_ACCESS, which this might change, right?
================
Comment at: lib/Sema/SemaChecking.cpp:10883
+ precision = AdjustPrecision(precision);
+ FloatValue.toString(PrettyTargetValue, precision);
+ IntValue.toString(PrettySourceValue);
----------------
I wonder if this call (finding the precision, 'adjusting' it, then writing to a
smallstring might be a better thing to pull into its own function rather than
AdjustPrecision.
https://reviews.llvm.org/D52835
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
