Skip to content

Commit

Permalink
Warn on duplicate optional statements generally instead of just MENU/…
Browse files Browse the repository at this point in the history 
…CLASS

This also moves the warning from the compiler to the parser
  • Loading branch information
@squeek502
squeek502 committed Jul 15, 2024
1 parent 309b0cf commit d63406f
Show file tree
Hide file tree
Showing 5 changed files with 91 additions and 33 deletions.
24 changes: 0 additions & 24 deletions src/compile.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1395,8 +1395,6 @@ pub const Compiler = struct {
menu.deinit(self.allocator);
}
}
var skipped_menu_or_classes = std.ArrayList(*Node.SimpleStatement).init(self.allocator);
defer skipped_menu_or_classes.deinit();
var last_menu: *Node.SimpleStatement = undefined;
var last_class: *Node.SimpleStatement = undefined;
var last_menu_would_be_forced_ordinal = false;
Expand Down Expand Up @@ -1426,9 +1424,6 @@ pub const Compiler = struct {
},
.class => {
const is_duplicate = optional_statement_values.class != null;
if (is_duplicate) {
try skipped_menu_or_classes.append(last_class);
}
const forced_ordinal = is_duplicate and optional_statement_values.class.? == .ordinal;
// In the Win32 RC compiler, if any CLASS values that are interpreted as
// an ordinal exist, it affects all future CLASS statements and forces
Expand Down Expand Up @@ -1456,9 +1451,6 @@ pub const Compiler = struct {
},
.menu => {
const is_duplicate = optional_statement_values.menu != null;
if (is_duplicate) {
try skipped_menu_or_classes.append(last_menu);
}
const forced_ordinal = is_duplicate and optional_statement_values.menu.? == .ordinal;
// In the Win32 RC compiler, if any MENU values that are interpreted as
// an ordinal exist, it affects all future MENU statements and forces
Expand Down Expand Up @@ -1542,22 +1534,6 @@ pub const Compiler = struct {
}
}

for (skipped_menu_or_classes.items) |simple_statement| {
const statement_identifier = simple_statement.identifier;
const statement_type = rc.OptionalStatements.dialog_map.get(statement_identifier.slice(self.source)) orelse continue;
try self.addErrorDetails(.{
.err = .duplicate_menu_or_class_skipped,
.type = .warning,
.token = simple_statement.identifier,
.token_span_start = simple_statement.base.getFirstToken(),
.token_span_end = simple_statement.base.getLastToken(),
.extra = .{ .menu_or_class = switch (statement_type) {
.menu => .menu,
.class => .class,
else => unreachable,
} },
});
}
// The Win32 RC compiler miscompiles the value in the following scenario:
// Multiple CLASS parameters are specified and any of them are treated as a number, then
// the last CLASS is always treated as a number no matter what
Expand Down
10 changes: 3 additions & 7 deletions src/errors.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -379,8 +379,7 @@ pub const ErrorDetails = struct {
rc_would_miscompile_dialog_menu_or_class_id_forced_ordinal,
rc_would_miscompile_dialog_menu_id_starts_with_digit,
dialog_menu_id_was_uppercased,
/// `menu_or_class` is populated and contains the type of the parameter statement
duplicate_menu_or_class_skipped,
duplicate_optional_statement_skipped,
invalid_digit_character_in_ordinal,

// Literals
Expand Down Expand Up @@ -740,11 +739,8 @@ pub const ErrorDetails = struct {
.hint => return,
},
.dialog_menu_id_was_uppercased => return,
.duplicate_menu_or_class_skipped => {
return writer.print("this {s} was ignored; when multiple {s} statements are specified, only the last takes precedence", .{
@tagName(self.extra.menu_or_class),
@tagName(self.extra.menu_or_class),
});
.duplicate_optional_statement_skipped => {
return writer.writeAll("this statement was ignored; when multiple statements of the same type are specified, only the last takes precedence");
},
.invalid_digit_character_in_ordinal => {
return writer.writeAll("non-ASCII digit characters are not allowed in ordinal (number) values");
Expand Down
46 changes: 46 additions & 0 deletions src/parse.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@ pub const Parser = struct {
/// The returned slice is allocated by the parser's arena
fn parseOptionalStatements(self: *Self, resource: Resource) ![]*Node {
var optional_statements = std.ArrayListUnmanaged(*Node){};

const num_statement_types = @typeInfo(rc.OptionalStatements).Enum.fields.len;
var statement_type_has_duplicates = [_]bool{false} ** num_statement_types;
var last_statement_per_type = [_]?*Node{null} ** num_statement_types;

while (true) {
const lookahead_token = try self.lookaheadToken(.normal);
if (lookahead_token.id != .literal) break;
Expand All @@ -141,6 +146,12 @@ pub const Parser = struct {
else => break,
};
self.nextToken(.normal) catch unreachable;

const type_i = @intFromEnum(optional_statement_type);
if (last_statement_per_type[type_i] != null) {
statement_type_has_duplicates[type_i] = true;
}

switch (optional_statement_type) {
.language => {
const language = try self.parseLanguageStatement();
Expand Down Expand Up @@ -272,7 +283,42 @@ pub const Parser = struct {
try optional_statements.append(self.state.arena, &node.base);
},
}

last_statement_per_type[type_i] = optional_statements.items[optional_statements.items.len - 1];
}

for (optional_statements.items) |optional_statement| {
const type_i = type_i: {
switch (optional_statement.id) {
.simple_statement => {
const simple_statement: *Node.SimpleStatement = @alignCast(@fieldParentPtr("base", optional_statement));
const statement_identifier = simple_statement.identifier;
const slice = statement_identifier.slice(self.lexer.buffer);
const optional_statement_type = rc.OptionalStatements.map.get(slice) orelse
rc.OptionalStatements.dialog_map.get(slice).?;
break :type_i @intFromEnum(optional_statement_type);
},
.font_statement => {
break :type_i @intFromEnum(rc.OptionalStatements.font);
},
.language_statement => {
break :type_i @intFromEnum(rc.OptionalStatements.language);
},
else => unreachable,
}
};
if (!statement_type_has_duplicates[type_i]) continue;
if (optional_statement == last_statement_per_type[type_i].?) continue;

try self.addErrorDetails(.{
.err = .duplicate_optional_statement_skipped,
.type = .warning,
.token = optional_statement.getFirstToken(),
.token_span_start = optional_statement.getFirstToken(),
.token_span_end = optional_statement.getLastToken(),
});
}

return optional_statements.toOwnedSlice(self.state.arena);
}

Expand Down
4 changes: 2 additions & 2 deletions test/compile.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -683,8 +683,8 @@ test "dialog, dialogex resource" {
);
try testCompileErrorDetailsWithDir(
&.{
.{ .type = .warning, .str = "this class was ignored; when multiple class statements are specified, only the last takes precedence" },
.{ .type = .warning, .str = "this class was ignored; when multiple class statements are specified, only the last takes precedence" },
.{ .type = .warning, .str = "this statement was ignored; when multiple statements of the same type are specified, only the last takes precedence" },
.{ .type = .warning, .str = "this statement was ignored; when multiple statements of the same type are specified, only the last takes precedence" },
.{ .type = .warning, .str = "this class would be miscompiled by the Win32 RC compiler" },
.{ .type = .note, .str = "the Win32 RC compiler would evaluate it as the ordinal/number value 62790" },
.{ .type = .note, .str = "to avoid the potential miscompilation, only specify one class per dialog resource" },
Expand Down
40 changes: 40 additions & 0 deletions test/parse.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2178,6 +2178,46 @@ test "language with L suffixed part" {
);
}

test "duplicate optional statements" {
try testParseErrorDetails(
&.{
.{ .type = .warning, .str = "this statement was ignored; when multiple statements of the same type are specified, only the last takes precedence" },
.{ .type = .warning, .str = "this statement was ignored; when multiple statements of the same type are specified, only the last takes precedence" },
.{ .type = .warning, .str = "this statement was ignored; when multiple statements of the same type are specified, only the last takes precedence" },
.{ .type = .warning, .str = "this statement was ignored; when multiple statements of the same type are specified, only the last takes precedence" },
.{ .type = .warning, .str = "this statement was ignored; when multiple statements of the same type are specified, only the last takes precedence" },
.{ .type = .warning, .str = "this statement was ignored; when multiple statements of the same type are specified, only the last takes precedence" },
.{ .type = .warning, .str = "this statement was ignored; when multiple statements of the same type are specified, only the last takes precedence" },
.{ .type = .warning, .str = "this statement was ignored; when multiple statements of the same type are specified, only the last takes precedence" },
.{ .type = .warning, .str = "this statement was ignored; when multiple statements of the same type are specified, only the last takes precedence" },
.{ .type = .warning, .str = "this statement was ignored; when multiple statements of the same type are specified, only the last takes precedence" },
},
\\1 DIALOGEX 1, 2, 3, 4
\\ CHARACTERISTICS 1
\\ LANGUAGE 0xFF, 0xFF
\\ LANGUAGE 0x09, 0x01
\\ CHARACTERISTICS 999
\\ CHARACTERISTICS 0x1234
\\ VERSION 999
\\ VERSION 1
\\ MENU 1
\\ MENU 2
\\ CLASS "foo"
\\ CLASS "bar"
\\ CAPTION "foo"
\\ CAPTION "bar"
\\ FONT 1, "foo"
\\ FONT 2, "bar"
\\ STYLE 1
\\ STYLE 2
\\ EXSTYLE 1
\\ EXSTYLE 2
\\{}
,
null,
);
}

fn testParse(source: []const u8, expected_ast_dump: []const u8) !void {
const allocator = std.testing.allocator;
var diagnostics = resinator.errors.Diagnostics.init(allocator);
Expand Down

0 comments on commit d63406f

Please sign in to comment.