Skip to content

Commit 800e8fd

Browse files
jonmeowjonmeow
authored
Add braces for CARBON_KIND uses that lack them (#5882)
Also document why these are expected on `CARBON_KIND`. In `kind_switch_test.cpp`, drop the `str` variable. My recollection of the original discussion of `CARBON_KIND` is that it should always have braces due to the risk of confusion for statement interpretation, similar to a typical `if`/`else` but more subtle due to the macro. For example: ``` case CARBON_KIND(int n): str << "int = " << n; return str.TakeStr(); ``` is equivalent to: ``` case CARBON_KIND(int n): { str << "int = " << n; } return str.TakeStr(); ``` This happens to work in context because `str` isn't scoped, but a trivial refactoring to move `RawStringOstream str;` the first statement of the `case` would probably have non-obvious results. For example: ``` case CARBON_KIND(int n): RawStringOstream str; // Valid name shadowing, destructed without use. str << "int = " << n; // Name lookup error on `n`. return str.TakeStr(); ```
1 parent 4c0979f commit 800e8fd

File tree

6 files changed

+83
-49
lines changed

6 files changed

+83
-49
lines changed

‎toolchain/base/kind_switch.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,11 @@ auto Cast(SwitchT&& kind_switch_value) -> decltype(auto) {
253253
// This uses `if` to scope the variable, and provides a dangling `else` in order
254254
// to prevent accidental `else` use. The label allows `:` to follow the macro
255255
// name, making it look more like a typical `case`.
256+
//
257+
// Because of the dangling else, braces should always be used with a `case
258+
// CARBON_KIND`. Otherwise, only the first statement is going to see the
259+
// variable. Even if that sometimes works, it can lead to confusing issues when
260+
// statements are added, and `if`/`else` coding style already requires braces.
256261
#define CARBON_KIND(typed_variable_decl) \
257262
::Carbon::Internal::Kind::ForCase< \
258263
decltype(carbon_internal_kind_switch_value), \

‎toolchain/base/kind_switch_test.cpp

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -16,36 +16,33 @@ namespace {
1616

1717
TEST(KindSwitch, Variant) {
1818
auto f = [](std::variant<int, float, char> v) -> std::string {
19-
RawStringOstream str;
2019
CARBON_KIND_SWITCH(v) {
21-
case CARBON_KIND(int n):
22-
str << "int = " << n;
23-
return str.TakeStr();
24-
case CARBON_KIND(float f):
25-
str << "float = " << f;
26-
return str.TakeStr();
27-
case CARBON_KIND(char c):
28-
str << "char = " << c;
29-
return str.TakeStr();
20+
case CARBON_KIND(int n): {
21+
return llvm::formatv("int = {0}", n);
22+
}
23+
case CARBON_KIND(float f): {
24+
return llvm::formatv("float = {0}", f);
25+
}
26+
case CARBON_KIND(char c): {
27+
return llvm::formatv("char = {0}", c);
28+
}
3029
}
3130
};
3231

3332
EXPECT_EQ(f(int{1}), "int = 1");
34-
EXPECT_EQ(f(float{2}), "float = 2.000000e+00");
33+
EXPECT_EQ(f(float{2}), "float = 2.00");
3534
EXPECT_EQ(f(char{'h'}), "char = h");
3635
}
3736

3837
TEST(KindSwitch, VariantUnusedValue) {
3938
auto f = [](std::variant<int, float> v) -> std::string {
40-
RawStringOstream str;
4139
CARBON_KIND_SWITCH(v) {
42-
case CARBON_KIND(int n):
43-
str << "int = " << n;
44-
return str.TakeStr();
40+
case CARBON_KIND(int n): {
41+
return llvm::formatv("int = {0}", n);
42+
}
4543
case CARBON_KIND(float _):
4644
// The float value is not used, we see that using `_` works.
47-
str << "float";
48-
return str.TakeStr();
45+
return "float";
4946
}
5047
};
5148

‎toolchain/check/impl_validation.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,8 +92,9 @@ static auto DiagnoseFinalImplNotInSameFileAsRootSelfTypeOrInterface(
9292
break;
9393
}
9494

95-
case CARBON_KIND(Step::Done _):
95+
case CARBON_KIND(Step::Done _): {
9696
CARBON_FATAL("self type is empty?");
97+
}
9798

9899
default:
99100
break;

‎toolchain/check/type_structure.cpp

Lines changed: 38 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -206,67 +206,86 @@ auto TypeStructureBuilder::Build(SemIR::TypeIterator type_iter)
206206
while (true) {
207207
using Step = SemIR::TypeIterator::Step;
208208
CARBON_KIND_SWITCH(type_iter.Next().any) {
209-
case CARBON_KIND(Step::Done _):
209+
case CARBON_KIND(Step::Done _): {
210210
// TODO: This requires 4 SmallVector moves (two here and two in the
211211
// constructor). Find a way to reduce that.
212212
return TypeStructure(std::exchange(structure_, {}),
213213
std::exchange(symbolic_type_indices_, {}),
214214
std::exchange(concrete_types_, {}));
215-
case CARBON_KIND(Step::End _):
215+
}
216+
case CARBON_KIND(Step::End _): {
216217
AppendStructuralConcreteCloseParen();
217218
break;
218-
case CARBON_KIND(Step::ConcreteType concrete):
219+
}
220+
case CARBON_KIND(Step::ConcreteType concrete): {
219221
AppendStructuralConcrete(concrete.type_id);
220222
break;
221-
case CARBON_KIND(Step::SymbolicType _):
223+
}
224+
case CARBON_KIND(Step::SymbolicType _): {
222225
AppendStructuralSymbolic();
223226
break;
224-
case CARBON_KIND(Step::TemplateType _):
227+
}
228+
case CARBON_KIND(Step::TemplateType _): {
225229
AppendStructuralSymbolic();
226230
break;
227-
case CARBON_KIND(Step::ConcreteValue value):
231+
}
232+
case CARBON_KIND(Step::ConcreteValue value): {
228233
AppendStructuralConcrete(
229234
context_->constant_values().Get(value.inst_id));
230235
break;
231-
case CARBON_KIND(Step::SymbolicValue _):
236+
}
237+
case CARBON_KIND(Step::SymbolicValue _): {
232238
AppendStructuralSymbolic();
233239
break;
234-
case CARBON_KIND(Step::StructFieldName field_name):
240+
}
241+
case CARBON_KIND(Step::StructFieldName field_name): {
235242
AppendStructuralConcrete(field_name.name_id);
236243
break;
237-
case CARBON_KIND(Step::ClassStartOnly class_start):
244+
}
245+
case CARBON_KIND(Step::ClassStartOnly class_start): {
238246
AppendStructuralConcrete(class_start.class_id);
239247
break;
240-
case CARBON_KIND(Step::ClassStart class_start):
248+
}
249+
case CARBON_KIND(Step::ClassStart class_start): {
241250
AppendStructuralConcreteOpenParen(class_start.class_id);
242251
break;
243-
case CARBON_KIND(Step::StructStartOnly _):
252+
}
253+
case CARBON_KIND(Step::StructStartOnly _): {
244254
AppendStructuralConcrete(TypeStructure::ConcreteStructType());
245255
break;
246-
case CARBON_KIND(Step::StructStart _):
256+
}
257+
case CARBON_KIND(Step::StructStart _): {
247258
AppendStructuralConcreteOpenParen(TypeStructure::ConcreteStructType());
248259
break;
249-
case CARBON_KIND(Step::TupleStartOnly _):
260+
}
261+
case CARBON_KIND(Step::TupleStartOnly _): {
250262
AppendStructuralConcrete(TypeStructure::ConcreteTupleType());
251263
break;
252-
case CARBON_KIND(Step::TupleStart _):
264+
}
265+
case CARBON_KIND(Step::TupleStart _): {
253266
AppendStructuralConcreteOpenParen(TypeStructure::ConcreteTupleType());
254267
break;
255-
case CARBON_KIND(Step::InterfaceStartOnly interface_start):
268+
}
269+
case CARBON_KIND(Step::InterfaceStartOnly interface_start): {
256270
AppendStructuralConcrete(interface_start.interface_id);
257271
break;
258-
case CARBON_KIND(Step::InterfaceStart interface_start):
272+
}
273+
case CARBON_KIND(Step::InterfaceStart interface_start): {
259274
AppendStructuralConcreteOpenParen(interface_start.interface_id);
260275
break;
261-
case CARBON_KIND(Step::IntStart int_start):
276+
}
277+
case CARBON_KIND(Step::IntStart int_start): {
262278
AppendStructuralConcreteOpenParen(int_start.type_id);
263279
break;
264-
case CARBON_KIND(Step::ArrayStart _):
280+
}
281+
case CARBON_KIND(Step::ArrayStart _): {
265282
AppendStructuralConcreteOpenParen(TypeStructure::ConcreteArrayType());
266283
break;
267-
case CARBON_KIND(Step::PointerStart _):
284+
}
285+
case CARBON_KIND(Step::PointerStart _): {
268286
AppendStructuralConcreteOpenParen(TypeStructure::ConcretePointerType());
269287
break;
288+
}
270289
}
271290
}
272291
}

‎toolchain/lex/lex.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1105,12 +1105,13 @@ auto Lexer::LexNumericLiteral(llvm::StringRef source_text, ssize_t& position)
11051105
position += token_size;
11061106

11071107
CARBON_KIND_SWITCH(literal->ComputeValue(emitter_)) {
1108-
case CARBON_KIND(NumericLiteral::IntValue && value):
1108+
case CARBON_KIND(NumericLiteral::IntValue && value): {
11091109
return LexTokenWithPayload(TokenKind::IntLiteral,
11101110
buffer_.value_stores_->ints()
11111111
.AddUnsigned(std::move(value.value))
11121112
.AsTokenPayload(),
11131113
byte_offset);
1114+
}
11141115
case CARBON_KIND(NumericLiteral::RealValue && value): {
11151116
auto real_id = buffer_.value_stores_->reals().Add(
11161117
Real{.mantissa = value.mantissa,
@@ -1119,8 +1120,9 @@ auto Lexer::LexNumericLiteral(llvm::StringRef source_text, ssize_t& position)
11191120
return LexTokenWithPayload(TokenKind::RealLiteral, real_id.index,
11201121
byte_offset);
11211122
}
1122-
case CARBON_KIND(NumericLiteral::UnrecoverableError _):
1123+
case CARBON_KIND(NumericLiteral::UnrecoverableError _): {
11231124
return LexTokenWithPayload(TokenKind::Error, token_size, byte_offset);
1125+
}
11241126
}
11251127
}
11261128

‎toolchain/sem_ir/stringify.cpp

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -123,36 +123,46 @@ class StepStack {
123123
auto PushArray(llvm::ArrayRef<PushItem> items) -> void {
124124
for (auto item : llvm::reverse(items)) {
125125
CARBON_KIND_SWITCH(item) {
126-
case CARBON_KIND(InstId inst_id):
126+
case CARBON_KIND(InstId inst_id): {
127127
PushInstId(inst_id);
128128
break;
129-
case CARBON_KIND(llvm::StringRef string):
129+
}
130+
case CARBON_KIND(llvm::StringRef string): {
130131
PushString(string);
131132
break;
132-
case CARBON_KIND(NameId name_id):
133+
}
134+
case CARBON_KIND(NameId name_id): {
133135
PushNameId(name_id);
134136
break;
135-
case CARBON_KIND(ElementIndex element_index):
137+
}
138+
case CARBON_KIND(ElementIndex element_index): {
136139
PushElementIndex(element_index);
137140
break;
138-
case CARBON_KIND(QualifiedNameItem qualified_name):
141+
}
142+
case CARBON_KIND(QualifiedNameItem qualified_name): {
139143
PushQualifiedName(qualified_name.first, qualified_name.second);
140144
break;
141-
case CARBON_KIND(EntityNameItem entity_name):
145+
}
146+
case CARBON_KIND(EntityNameItem entity_name): {
142147
PushEntityName(entity_name.first, entity_name.second);
143148
break;
144-
case CARBON_KIND(EntityNameId entity_name_id):
149+
}
150+
case CARBON_KIND(EntityNameId entity_name_id): {
145151
PushEntityNameId(entity_name_id);
146152
break;
147-
case CARBON_KIND(TypeId type_id):
153+
}
154+
case CARBON_KIND(TypeId type_id): {
148155
PushTypeId(type_id);
149156
break;
150-
case CARBON_KIND(SpecificInterface specific_interface):
157+
}
158+
case CARBON_KIND(SpecificInterface specific_interface): {
151159
PushSpecificInterface(specific_interface);
152160
break;
153-
case CARBON_KIND(llvm::ListSeparator * sep):
161+
}
162+
case CARBON_KIND(llvm::ListSeparator * sep): {
154163
PushString(*sep);
155164
break;
165+
}
156166
}
157167
}
158168
}

0 commit comments

Comments
 (0)