surface: a generated name may not be a C++ keyword

Six sites across four files ran the same character filter -- letters, digits
and `_` survive, everything else becomes `_`, a leading digit gets a `_` --
and none of them asked whether the result is reserved. `default`,
`template`, `operator`, `private` and `union` are valid identifiers to that
filter, and `shaders/default/` is an ordinary name for a shader directory.

Measured: renaming the fixture's `shaders/a` to `shaders/default` makes the
generator write

    namespace default {

and the build fails at `expected identifier before 'default'`, inside a
generated file, on a line the author of that directory has never opened.

`surface::identifier` does all three transformations in one place, and a
reserved result gets a TRAILING underscore -- a leading one is itself
reserved at namespace scope, so prefixing would trade one reserved name for
another. Namespace segments in `rules-spirv` and `rules-slang` go through
it, as does `tools-embed`'s accessor name.

The accessor is sanitised where it is EMITTED rather than in each producer.
A rule that builds `item::identifier` from a file stem cannot know it has
produced `my-shader_comp` or `default` until it reaches the one line that
writes the function's name. `accessor_base` and the `_spv` symbols are left
alone: both carry fixed affixes and cannot come out reserved, and routing
them would rename symbols already published.

The fixture could not see any of this: `a` and `b` are ordinary identifiers,
so both filters produce the same file. It gains `shaders/default/`, and CI
asserts the generated interface contains `namespace default_ {`.

Author: Sunrisepeak

.github/workflows/ci.yml

@@ -115,7 +115,17 @@ jobs:
           "$MCPP" run | tee run.log
           grep -q '^a/scale.comp: magic=07230203' run.log
           grep -q '^b/scale.comp: magic=07230203' run.log
+          grep -q '^default/scale.comp: magic=07230203' run.log
           grep -q '^all ok' run.log
+          # A DIRECTORY NAMED AFTER A C++ KEYWORD. `a` and `b` are ordinary
+          # identifiers and cannot tell a name filter that knows the keywords
+          # from one that does not; without the guard this is
+          # `namespace default {` and the generated file does not parse.
+          grep -q '^namespace default_ {' \
+            target/.build-mcpp/out/spirv/shader_app.shaders.cppm \
+            || { echo "FAIL: the keyword directory did not get its trailing underscore"
+                 grep -n namespace target/.build-mcpp/out/spirv/shader_app.shaders.cppm
+                 exit 1; }
           # The point of the surface: a consumer names no generated file.
           if grep -rn 'scale_comp\.h\|\.inc"' src/; then
             echo "FAIL: a consumer source names a generated file"

rules/slang.cppm

@@ -267,11 +267,10 @@ inline std::vector<std::string> namespace_of(std::string_view src, std::string_v
     for (auto const& part : std::filesystem::path(dir)) {
         auto s = part.string();
         if (s.empty() || s == "." || s == "/") continue;
-        std::string seg;
-        for (char c : s)
-            seg += (std::isalnum(static_cast<unsigned char>(c)) || c == '_') ? c : '_';
-        if (std::isdigit(static_cast<unsigned char>(seg.front()))) seg.insert(seg.begin(), '_');
-        out.push_back(std::move(seg));
+        // Through the lib root, which is the one place that knows a segment
+        // may not be a keyword: `shaders/default/` is an ordinary directory
+        // name and `namespace default {` is not a namespace.
+        out.push_back(mcpp::plugins::surface::identifier(s, "dir"));
     }
     return out;
 }

rules/spirv.cppm

@@ -477,11 +477,10 @@ inline std::vector<std::string> namespace_of(std::string_view src, std::string_v
     for (auto const& part : std::filesystem::path(dir)) {
         auto s = part.string();
         if (s.empty() || s == "." || s == "/") continue;
-        std::string seg;
-        for (char c : s)
-            seg += (std::isalnum(static_cast<unsigned char>(c)) || c == '_') ? c : '_';
-        if (std::isdigit(static_cast<unsigned char>(seg.front()))) seg.insert(seg.begin(), '_');
-        out.push_back(std::move(seg));
+        // Through the lib root, which is the one place that knows a segment
+        // may not be a keyword: `shaders/default/` is an ordinary directory
+        // name and `namespace default {` is not a namespace.
+        out.push_back(mcpp::plugins::surface::identifier(s, "dir"));
     }
     return out;
 }

src/plugins.cppm

@@ -267,6 +267,68 @@ inline std::string accessor_base(const options& opt, const item& it) {
     return s;
 }
 
+// A GENERATED NAME THE C++ COMPILER WILL ACCEPT.
+//
+// Three transformations, and the third is the one every hand-rolled copy of
+// this function was missing. Non-identifier characters become `_`; a leading
+// digit gets a `_` in front; and a result that is a KEYWORD gets a trailing `_`.
+//
+// The keyword case is not hypothetical. The first two rules accept `default`,
+// `template`, `operator`, `private` and `union` unchanged -- they are valid
+// identifiers to a character filter and reserved to the compiler -- and
+// `shaders/default/` is an ordinary name for a shader directory. What it
+// produced was `namespace default {` in a generated file, and an error naming a
+// line its author never wrote.
+//
+// TRAILING `_`, not a prefix: `_default` is reserved at namespace scope
+// (a leading underscore in the global namespace), and prefixing would trade one
+// reserved name for another.
+//
+// The list is the keywords of the standard this collection targets. A word that
+// is contextual rather than reserved (`final`, `override`, `import`, `module`)
+// is a legal identifier and is left alone.
+inline bool is_cxx_keyword(std::string_view w) {
+    static constexpr std::string_view kWords[] = {
+        "alignas", "alignof", "and", "and_eq", "asm", "auto", "bitand", "bitor",
+        "bool", "break", "case", "catch", "char", "char8_t", "char16_t",
+        "char32_t", "class", "compl", "concept", "const", "consteval",
+        "constexpr", "constinit", "const_cast", "continue", "co_await",
+        "co_return", "co_yield", "decltype", "default", "delete", "do", "double",
+        "dynamic_cast", "else", "enum", "explicit", "export", "extern", "false",
+        "float", "for", "friend", "goto", "if", "inline", "int", "long",
+        "mutable", "namespace", "new", "noexcept", "not", "not_eq", "nullptr",
+        "operator", "or", "or_eq", "private", "protected", "public", "register",
+        "reinterpret_cast", "requires", "return", "short", "signed", "sizeof",
+        "static", "static_assert", "static_cast", "struct", "switch",
+        "template", "this", "thread_local", "throw", "true", "try", "typedef",
+        "typeid", "typename", "union", "unsigned", "using", "virtual", "void",
+        "volatile", "wchar_t", "while", "xor", "xor_eq",
+    };
+    for (auto k : kWords) if (k == w) return true;
+    return false;
+}
+
+// `fallback` is used when the input sanitises to nothing, which a file named
+// only in punctuation does.
+//
+// INDEXED RATHER THAN A RANGE-FOR over the string, for the reason recorded in
+// `mcpp.tools.island`: iterating a `std::string` inside an exported inline
+// function makes GCC 16 instantiate its iterator in this BMI, and a consumer's
+// build program then fails to compile on `always_inline` in a header naming
+// neither this file nor this loop.
+inline std::string identifier(std::string_view raw, std::string_view fallback) {
+    std::string s;
+    for (std::size_t i = 0; i < raw.size(); ++i) {
+        const char c = raw[i];
+        s += (std::isalnum(static_cast<unsigned char>(c)) || c == '_') ? c : '_';
+    }
+    if (s.empty()) s = std::string(fallback);
+    if (!s.empty() && std::isdigit(static_cast<unsigned char>(s.front())))
+        s.insert(s.begin(), '_');
+    if (is_cxx_keyword(s)) s += '_';
+    return s;
+}
+
 inline const char* element_type(element e) {
     return e == element::word32 ? "unsigned int" : "unsigned char";
 }
@@ -354,8 +416,12 @@ inline std::string declarations(std::span<const item> items, const options& opt)
     for (auto const& it : items) {
         reopen(it.name_space);
         const auto base = accessor_base(opt, it);
+        // THROUGH `identifier`, HERE RATHER THAN IN EACH PRODUCER. This is the
+        // one line that turns `item::identifier` into something a compiler
+        // parses, and a rule that builds the field from a file stem cannot know
+        // it has produced `my-shader_comp` or `default` until it gets here.
         s += std::format("inline payload {}() {{ return {{ {}_data(), {}_size() }}; }}\n",
-                         it.identifier, base, base);
+                         identifier(it.identifier, "payload"), base, base);
     }
     reopen({});
     return s;
@@ -646,12 +712,7 @@ inline std::string module_root_from_package() {
     std::string leaf{mcpp::package_name()};
     if (leaf.empty())
         leaf = std::filesystem::path(mcpp::manifest_dir()).filename().string();
-    std::string s;
-    for (char c : leaf)
-        s += (std::isalnum(static_cast<unsigned char>(c)) || c == '_') ? c : '_';
-    if (s.empty()) s = "app";
-    if (std::isdigit(static_cast<unsigned char>(s.front()))) s.insert(s.begin(), '_');
-    return s;
+    return identifier(leaf, "app");
 }
 
 } // namespace mcpp::plugins::surface

tests/spirv-module-consumer/mcpp.toml

@@ -6,7 +6,11 @@
 #     header anywhere in its own sources;
 #   - two shaders with one stem in two directories are no longer a collision:
 #     `shaders/a/scale.comp` and `shaders/b/scale.comp` land in
-#     `spirv_module_consumer::shaders::a` and `::b`;
+#     `shader_app::shaders::a` and `::b`;
+#   - a directory whose name is a C++ KEYWORD is reached as `default_`. `a` and
+#     `b` are ordinary identifiers and cannot tell a name filter that knows the
+#     keywords from one that does not; `shaders/default/` can, and without the
+#     guard the generator writes `namespace default {`;
 #   - the module name and the namespace are the same identifier path, derived
 #     from the package with nothing declared for it.
 #

tests/spirv-module-consumer/shaders/default/scale.comp

@@ -0,0 +1,15 @@
+#version 450
+
+// The device side of the same computation `examples/09-cuda-kernel` runs on
+// CUDA: out = a*x + y. One storage buffer holds all three vectors so the host
+// side needs one allocation and one descriptor.
+layout(local_size_x = 64) in;
+
+layout(std430, binding = 0) buffer Data { float v[]; };
+layout(push_constant) uniform Push { float a; uint n; } push;
+
+void main() {
+    const uint i = gl_GlobalInvocationID.x;
+    if (i >= push.n) return;
+    v[2u * push.n + i] = push.a * v[i] + v[push.n + i];
+}

tests/spirv-module-consumer/src/main.cpp

@@ -28,6 +28,14 @@ int main() {
     bool ok = true;
     ok &= check("a/scale.comp", shaders::a::scale_comp());
     ok &= check("b/scale.comp", shaders::b::scale_comp());
+    // A DIRECTORY NAMED AFTER A C++ KEYWORD, reached as `default_`.
+    //
+    // `a` and `b` are ordinary identifiers, so neither could tell a name filter
+    // that knows the keywords from one that does not. `shaders/default/` is an
+    // ordinary name for a shader directory and `namespace default {` is not a
+    // namespace: without the trailing underscore the generator writes a file
+    // that does not parse, and the error names a line nobody wrote.
+    ok &= check("default/scale.comp", shaders::default_::scale_comp());
 
     // The two shaders are the same source in two directories, so they must
     // produce identical modules -- and they must be two distinct objects, not

tools/embed.cppm

@@ -89,13 +89,11 @@ struct options {
 
 // ---- internals -------------------------------------------------------------
 
+// The accessor's own name, so a file called `default.bin` must not produce
+// `default()`. The lib root owns that decision; this is the one caller that
+// needs it here.
 inline std::string sanitise(std::string_view stem) {
-    std::string s;
-    for (char c : stem)
-        s += (std::isalnum(static_cast<unsigned char>(c)) || c == '_') ? c : '_';
-    if (s.empty()) s = "data";
-    if (std::isdigit(static_cast<unsigned char>(s.front()))) s.insert(s.begin(), '_');
-    return s;
+    return mcpp::plugins::surface::identifier(stem, "data");
 }
 
 inline std::string default_dir() {