Apply review suggestions

Author: ivanaivanovska

toolchain/check/cpp/import.cpp

@@ -30,7 +30,9 @@
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/raw_ostream.h"
+#include "toolchain/base/int.h"
 #include "toolchain/base/kind_switch.h"
+#include "toolchain/base/value_ids.h"
 #include "toolchain/check/call.h"
 #include "toolchain/check/class.h"
 #include "toolchain/check/context.h"
@@ -2247,8 +2249,8 @@ static auto IsIncompleteClass(Context& context, SemIR::NameScopeId scope_id)
              context.classes().Get(class_decl->class_id).self_type_id);
 }
 
-// Maps a Clang constant expression to a Carbon constant. Currently supports
-// only integer constants.
+// Maps a Clang literal expression to a Carbon constant. Currently supports
+// only integer and floating-point literals.
 // TODO: Add support for the other constant types for which a C++ to Carbon type
 // mapping exists.
 static auto MapConstant(Context& context, SemIR::LocId loc_id,
@@ -2261,24 +2263,31 @@ static auto MapConstant(Context& context, SemIR::LocId loc_id,
         loc_id, "Unsupported: constant type: " + expr->getType().getAsString());
     return SemIR::ErrorInst::InstId;
   }
+
   SemIR::InstId inst_id = SemIR::InstId::None;
+  SemIR::ImportIRInstId imported_loc_id =
+      AddImportIRInst(context.sem_ir(), expr->getExprLoc());
+
   if (auto* integer_literal = dyn_cast<clang::IntegerLiteral>(expr)) {
-    auto int_id =
+    IntId int_id =
         context.ints().Add(integer_literal->getValue().getSExtValue());
-    inst_id = AddInstInNoBlock<SemIR::IntValue>(
-        context, loc_id, {.type_id = type_id, .int_id = int_id});
+    inst_id = AddInstInNoBlock(
+        context,
+        MakeImportedLocIdAndInst<SemIR::IntValue>(
+            context, imported_loc_id, {.type_id = type_id, .int_id = int_id}));
   } else if (auto* float_literal = dyn_cast<clang::FloatingLiteral>(expr)) {
-    auto float_id = context.floats().Add(float_literal->getValue());
-    inst_id = AddInstInNoBlock<SemIR::FloatValue>(
-        context, loc_id, {.type_id = type_id, .float_id = float_id});
+    FloatId float_id = context.floats().Add(float_literal->getValue());
+    inst_id = AddInstInNoBlock(context,
+                               MakeImportedLocIdAndInst<SemIR::FloatValue>(
+                                   context, imported_loc_id,
+                                   {.type_id = type_id, .float_id = float_id}));
   } else {
     context.TODO(
         loc_id, "Unsupported: constant type: " + expr->getType().getAsString());
-    inst_id = SemIR::ErrorInst::InstId;
-  }
-  if (inst_id != SemIR::ErrorInst::InstId) {
-    context.imports().push_back(inst_id);
+    return SemIR::ErrorInst::InstId;
   }
+
+  context.imports().push_back(inst_id);
   return inst_id;
 }
 

toolchain/check/cpp/macros.cpp

@@ -8,6 +8,7 @@
 #include "clang/AST/Expr.h"
 #include "clang/Parse/Parser.h"
 #include "clang/Sema/Sema.h"
+#include "common/check.h"
 
 namespace Carbon::Check {
 
@@ -54,25 +55,30 @@ auto TryEvaluateMacroToConstant(Context& context, SemIR::LocId loc_id,
     parser.SkipUntil(clang::tok::eof);
     CARBON_DIAGNOSTIC(
         InCppMacroEvaluation, Error,
-        "failed to evaluate macro Cpp.{0} to a valid constant expression",
+        "failed to parse macro Cpp.{0} to a valid constant expression",
         std::string);
     context.emitter().Emit(loc_id, InCppMacroEvaluation, (*name_str_opt).str());
     return nullptr;
   }
 
   clang::Expr::EvalResult evaluated_result;
-  if (result_expr->EvaluateAsInt(evaluated_result, sema.getASTContext())) {
+  if (!result_expr->EvaluateAsConstantExpr(evaluated_result,
+                                           sema.getASTContext())) {
+    context.TODO(loc_id,
+                 "failed to evaluate macro to a valid constant expression");
+    return nullptr;
+  }
+  clang::APValue ap_value = evaluated_result.Val;
+  if (ap_value.isInt()) {
     return clang::IntegerLiteral::Create(
-        sema.getASTContext(), evaluated_result.Val.getInt(),
-        result_expr->getType(), result_expr->getExprLoc());
+        sema.getASTContext(), ap_value.getInt(), result_expr->getType(),
+        result_expr->getExprLoc());
   }
-  llvm::APFloat ap_value(0.0);
-  if (result_expr->EvaluateAsFloat(ap_value, sema.getASTContext())) {
+  if (ap_value.isFloat()) {
     return clang::FloatingLiteral::Create(
-        sema.getASTContext(), ap_value,
+        sema.getASTContext(), ap_value.getFloat(),
         /*isExact=*/true, result_expr->getType(), result_expr->getExprLoc());
   }
-
   context.TODO(loc_id, "Unsupported: constant type:" +
                            result_expr->getType().getAsString());
   return nullptr;

toolchain/check/testdata/interop/cpp/macros.carbon

@@ -57,7 +57,7 @@ fn F() {
   // CHECK:STDERR:   Cpp.CONFIG_VALUE;
   // CHECK:STDERR:   ^~~~~~~~~~~~~~~~
   // CHECK:STDERR:
-  // CHECK:STDERR: fail_import_bad_suffix.carbon:[[@LINE+11]]:3: error: failed to evaluate macro Cpp.CONFIG_VALUE to a valid constant expression [InCppMacroEvaluation]
+  // CHECK:STDERR: fail_import_bad_suffix.carbon:[[@LINE+11]]:3: error: failed to parse macro Cpp.CONFIG_VALUE to a valid constant expression [InCppMacroEvaluation]
   // CHECK:STDERR:   Cpp.CONFIG_VALUE;
   // CHECK:STDERR:   ^~~~~~~~~~~~~~~~
   // CHECK:STDERR: fail_import_bad_suffix.carbon:[[@LINE+8]]:3: note: in `Cpp` name lookup for `CONFIG_VALUE` [InCppNameLookup]
@@ -321,6 +321,20 @@ fn F() {
   Cpp.MyLongDouble;
 }
 
+// --- fail_import_assign_to_float.carbon
+
+library "[[@TEST_NAME]]";
+
+import Cpp library "floating_point_literal_macro.h";
+
+fn F() {
+  // CHECK:STDERR: fail_import_assign_to_float.carbon:[[@LINE+4]]:3: error: expression is not assignable [AssignmentToNonAssignable]
+  // CHECK:STDERR:   Cpp.MyDouble = 1.0;
+  // CHECK:STDERR:   ^~~~~~~~~~~~
+  // CHECK:STDERR:
+  Cpp.MyDouble = 1.0;
+}
+
 // --- macro_undefined.h
 
 #define CONFIG_VALUE 1

toolchain/sem_ir/typed_insts.h

@@ -683,8 +683,10 @@ struct FloatType {
 
 // A floating point value.
 struct FloatValue {
-  static constexpr auto Kind = InstKind::FloatValue.Define<Parse::NodeId>(
-      {.ir_name = "float_value", .constant_kind = InstConstantKind::Always});
+  static constexpr auto Kind =
+      InstKind::FloatValue.Define<Parse::RealLiteralId>(
+          {.ir_name = "float_value",
+           .constant_kind = InstConstantKind::Always});
 
   TypeId type_id;
   FloatId float_id;