Skip to content

Commit ee719f1

Browse files
ammaralassalammaralassal
committed
Address feedback
1 parent f6111a8 commit ee719f1

File tree

5 files changed

+115
-30
lines changed

5 files changed

+115
-30
lines changed

‎toolchain/check/eval.cpp‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1687,6 +1687,63 @@ static auto MakeConstantForBuiltinCall(EvalContext& eval_context,
16871687
return context.constant_values().Get(arg_ids[0]);
16881688
}
16891689

1690+
case SemIR::BuiltinFunctionKind::StringAt: {
1691+
Phase phase = Phase::Concrete;
1692+
auto str_id = GetConstantValue(eval_context, arg_ids[0], &phase);
1693+
auto index_id = GetConstantValue(eval_context, arg_ids[1], &phase);
1694+
1695+
if (phase != Phase::Concrete) {
1696+
return MakeNonConstantResult(phase);
1697+
}
1698+
1699+
auto str_struct = eval_context.insts().GetAs<SemIR::StructValue>(str_id);
1700+
auto elements = eval_context.inst_blocks().Get(str_struct.elements_id);
1701+
CARBON_CHECK(elements.size() == 2, "String struct should have 2 fields.");
1702+
1703+
auto ptr_const_id = eval_context.constant_values().Get(elements[0]);
1704+
auto string_literal = eval_context.insts().GetAs<SemIR::StringLiteral>(
1705+
eval_context.constant_values().GetInstId(ptr_const_id));
1706+
1707+
auto string_value = eval_context.sem_ir().string_literal_values().Get(
1708+
string_literal.string_literal_id);
1709+
1710+
auto index_inst = eval_context.insts().GetAs<SemIR::IntValue>(index_id);
1711+
const auto& index_val = eval_context.ints().Get(index_inst.int_id);
1712+
1713+
// Check bounds
1714+
if (index_val.isNegative()) {
1715+
CARBON_DIAGNOSTIC(ArrayIndexNegative, Error, "index `{0}` is negative.",
1716+
TypedInt);
1717+
eval_context.emitter().Emit(
1718+
eval_context.GetDiagnosticLoc(index_id), ArrayIndexNegative,
1719+
{.type = eval_context.insts().Get(index_id).type_id(),
1720+
.value = index_val});
1721+
return SemIR::ErrorInst::ConstantId;
1722+
}
1723+
1724+
// Check for out of bounds
1725+
if (index_val.getActiveBits() > 64 ||
1726+
index_val.getZExtValue() >= string_value.size()) {
1727+
CARBON_DIAGNOSTIC(StringAtIndexOutOfBounds, Error,
1728+
"string index `{0}` is past the end of the string.",
1729+
TypedInt);
1730+
eval_context.emitter().Emit(
1731+
eval_context.GetDiagnosticLoc(index_id), StringAtIndexOutOfBounds,
1732+
{.type = eval_context.insts().Get(index_id).type_id(),
1733+
.value = index_val});
1734+
return SemIR::ErrorInst::ConstantId;
1735+
}
1736+
1737+
auto char_value =
1738+
static_cast<uint8_t>(string_value[index_val.getZExtValue()]);
1739+
1740+
auto int_id = eval_context.ints().Add(
1741+
llvm::APSInt(llvm::APInt(32, char_value), /*isUnsigned=*/false));
1742+
return MakeConstantResult(
1743+
eval_context.context(),
1744+
SemIR::IntValue{.type_id = call.type_id, .int_id = int_id}, phase);
1745+
}
1746+
16901747
case SemIR::BuiltinFunctionKind::PrintChar:
16911748
case SemIR::BuiltinFunctionKind::PrintInt:
16921749
case SemIR::BuiltinFunctionKind::StringAt:{

‎toolchain/check/testdata/operators/overloaded/string_indexing.carbon‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
2+
// Exceptions. See /LICENSE for license information.
3+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
4+
// INCLUDE-FILE: toolchain/testing/testdata/min_prelude/full.carbon
5+
// AUTOUPDATE
6+
// TIP: To test this file alone, run:
7+
// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/operators/overloaded/string_indexing.carbon
8+
// TIP: To dump output, run:
9+
// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/operators/overloaded/string_indexing.carbon
10+
11+
// --- fail_wrong_type.carbon
12+
13+
fn TestWrongType() {
14+
var x: i32 = 42;
15+
// CHECK:STDERR: fail_wrong_type.carbon:[[@LINE+4]]:22: error: cannot access member of interface `Core.IndexWith(Core.IntLiteral)` in type `i32` that does not implement that interface [MissingImplInMemberAccess]
16+
// CHECK:STDERR: var c: Core.Char = x[0];
17+
// CHECK:STDERR: ^~~~
18+
// CHECK:STDERR:
19+
var c: Core.Char = x[0];
20+
}
21+

‎toolchain/diagnostics/diagnostic_kind.def‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -451,6 +451,7 @@ CARBON_DIAGNOSTIC_KIND(NegativeIntInUnsignedType)
451451
CARBON_DIAGNOSTIC_KIND(NonConstantCallToCompTimeOnlyFunction)
452452
CARBON_DIAGNOSTIC_KIND(CompTimeOnlyFunctionHere)
453453
CARBON_DIAGNOSTIC_KIND(SelfOutsideImplicitParamList)
454+
CARBON_DIAGNOSTIC_KIND(StringAtIndexOutOfBounds)
454455
CARBON_DIAGNOSTIC_KIND(StringLiteralTooLong)
455456
CARBON_DIAGNOSTIC_KIND(StringLiteralTypeIncomplete)
456457
CARBON_DIAGNOSTIC_KIND(StringLiteralTypeUnexpected)

‎toolchain/lower/handle_call.cpp‎

Lines changed: 18 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -319,41 +319,30 @@ static auto HandleBuiltinCall(FunctionContext& context, SemIR::InstId inst_id,
319319
}
320320

321321
case SemIR::BuiltinFunctionKind::StringAt: {
322-
// Get the string argument
323322
auto string_inst_id = arg_ids[0];
324323
auto* string_arg = context.GetValue(string_inst_id);
325-
326-
// Check if this is a pointer to a String or a String value
327-
llvm::Value* string_value;
328-
if (string_arg->getType()->isPointerTy()) {
329-
auto string_type_id = context.GetTypeIdOfInst(string_inst_id);
330-
auto* string_type = context.GetType(string_type_id);
331-
string_value = context.builder().CreateLoad(
332-
string_type, string_arg, "string.load");
333-
} else {
334-
// Already have the struct value
335-
string_value = string_arg;
336-
}
337-
338-
// String is a struct with ptr and size fields
339-
auto* string_ptr_field = context.builder().CreateExtractValue(
340-
string_value, {0}, "string.ptr");
341-
342-
// Get the index value
324+
325+
auto string_type_id = context.GetTypeIdOfInst(string_inst_id);
326+
auto* string_type = context.GetType(string_type_id);
327+
auto* string_value =
328+
context.builder().CreateLoad(string_type, string_arg, "string.load");
329+
330+
auto* string_ptr_field =
331+
context.builder().CreateExtractValue(string_value, {0}, "string.ptr");
332+
343333
auto* index_value = context.GetValue(arg_ids[1]);
344-
345-
// Compute the address of the character at the given index
334+
346335
auto* char_ptr = context.builder().CreateInBoundsGEP(
347-
llvm::Type::getInt8Ty(context.llvm_context()),
348-
string_ptr_field, index_value, "string.char_ptr");
349-
336+
llvm::Type::getInt8Ty(context.llvm_context()), string_ptr_field,
337+
index_value, "string.char_ptr");
338+
350339
auto* char_i8 = context.builder().CreateLoad(
351-
llvm::Type::getInt8Ty(context.llvm_context()),
352-
char_ptr, "string.char");
353-
354-
// Extend to i32
340+
llvm::Type::getInt8Ty(context.llvm_context()), char_ptr,
341+
"string.char");
342+
355343
context.SetLocal(inst_id, context.builder().CreateZExt(
356-
char_i8, context.GetTypeOfInst(inst_id), "string.char.zext"));
344+
char_i8, context.GetTypeOfInst(inst_id),
345+
"string.char.zext"));
357346
return;
358347
}
359348

‎toolchain/sem_ir/builtin_function_kind.cpp‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,22 @@ struct AnyType {
177177
return true;
178178
}
179179
};
180+
// Constraint that checks if a type is Core.String.
181+
struct CoreStringType {
182+
static auto Check(const File& sem_ir, ValidateState& /*state*/,
183+
TypeId type_id) -> bool {
184+
auto type_inst_id = sem_ir.types().GetInstId(type_id);
185+
auto class_type = sem_ir.insts().TryGetAs<ClassType>(type_inst_id);
186+
if (!class_type) {
187+
// Not a string.
188+
return false;
189+
}
190+
191+
const auto& class_info = sem_ir.classes().Get(class_type->class_id);
192+
193+
return sem_ir.names().GetFormatted(class_info.name_id).str() == "String";
194+
}
195+
};
180196

181197
// Constraint that requires the type to be the type type.
182198
using Type = BuiltinType<TypeType::TypeInstId>;
@@ -324,7 +340,8 @@ constexpr BuiltinInfo ReadChar = {"read.char",
324340

325341
// Gets a character from a string at the given index.
326342
constexpr BuiltinInfo StringAt = {
327-
"string.at", ValidateSignature<auto(AnyType, AnySizedInt)->AnySizedInt>};
343+
"string.at",
344+
ValidateSignature<auto(CoreStringType, AnySizedInt)->AnySizedInt>};
328345

329346
// Returns the `Core.CharLiteral` type.
330347
constexpr BuiltinInfo CharLiteralMakeType = {"char_literal.make_type",

0 commit comments

Comments
 (0)