vkd3d-shader/hlsl: Improvements to builtin typenames handling.
Merge request reports
Activity
From patch 1/4:
+static const char * get_case_insensitive_typename(const char *name) +{ + static const char *names[] = + { + "dword", + "float", + "matrix", + "string", + "vector", + };
We'd want the names[] array itself to be const as well, right? (I.e., "static const char *const names[] = ...")
added 4 commits
- fc7b1907 - vkd3d-shader/hlsl: Support case-insensitive lookup for selected builtin types.
- 0f83dbeb - vkd3d-shader/hlsl: Use unsigned type for "dword" alias.
- 79214f6b - vkd3d-shader/hlsl: Initial support for string constants.
- 56b67c4d - vkd3d-shader/hlsl: Allow case-insenstive names for shader objects types.
Toggle commit listI don't think this approach is quite going to work. Consider the following shader:
typedef float2 Dword;
float4 main() : sv_target { DWORD f = 1.0; return f; }
It seems that redefining these types is legal (although it doesn't seem to be legal to redefine lowercase "dword" as a type?). E.g. that shader compiles as-is, but if the first line is changed to "DWORD" it will predictably fail (float2 to float4 implicit conversion).
It also seems that at least "vector" and "string" should be case-insensitive keywords rather than typedefs. That's mostly out of scope here, but it does mean that patch 3/4 is going in the wrong direction.
From patch 3/4:
@@ -3509,6 +3509,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl INT intval; FLOAT floatval; bool boolval;
- char *strval; char *name; DWORD modifiers; struct hlsl_ir_node *instr;
I'd just rename "name" rather than adding a new union value.
@@ -611,6 +611,7 @@ struct hlsl_ir_constant int32_t i; float f; double d;
-
const char *s;
This is going to need special handling when the constant is cloned or freed.
Yes, looks like it's more convoluted. "vector" and "string" are lowercase tokens that can't be used for variable names for instance. Doing "float2 sTring;" is valid, but "sTring" can't be used as a type after that. Reusing names of already declared variables as typenames is universally disallowed, so that's good.
added 18 commits
-
56b67c4d...02e3be81 - 15 commits from branch
wine:master
- 200b2cc8 - vkd3d-shader/hlsl: Support case-insensitive lookup for builtin 'dword' type.
- e7e6c6dc - vkd3d-shader/hlsl: Use unsigned type for the 'dword' alias.
- a790ca79 - vkd3d-shader/hlsl: Support case-insensitive lookup for builtin 'float' type.
Toggle commit list-
56b67c4d...02e3be81 - 15 commits from branch
Yes, looks like it's more convoluted. "vector" and "string" are lowercase tokens that can't be used for variable names for instance. Doing "float2 sTring;" is valid, but "sTring" can't be used as a type after that. Reusing names of already declared variables as typenames is universally disallowed, so that's good.
Hrm, I thought I tested redefining non-lower-case versions of those, but I must have done it wrong.
Anyway, this is still failing with the example I posted, as far as I can tell because hlsl_get_type's recursive call passes "false" rather than propagating the "case_insensitive" argument.
Also, from patch 1/3:
- for (i = 0; i < ARRAY_SIZE(names); ++i)
-
if (!ascii_strcasecmp(names[i], name)) return names[i];
Please put the "if" body on a new line, and use braces for multi-line bodies even if it's technically a single statement.
- if (case_insensitive && !scope->upper && (name = get_case_insensitive_typename(name)))
- {
-
if ((entry = rb_get(&scope->types, name)))
-
return RB_ENTRY_VALUE(entry, struct hlsl_type, scope_entry);
- }
It looks mildly wrong that this checks "scope->upper" but not "recursive". It works, because we never pass recursive == false but case_insensitive == true, but I think it'd be clearer just to move this below the recursive call or use "else" there.
@@ -4565,7 +4565,7 @@ type_no_void: } | KW_STRUCT TYPE_IDENTIFIER {
-
$$ = hlsl_get_type(ctx->cur_scope, $2, true);
-
$$ = hlsl_get_type(ctx->cur_scope, $2, true, false); if ($$->type != HLSL_CLASS_STRUCT) hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_REDEFINED, "\"%s\" redefined as a structure.", $2); vkd3d_free($2);
This lacks tests (and also feels a little suspicious, mostly because it contradicts the lexer).
added 4 commits
Toggle commit list543 543 } 544 544 545 struct hlsl_type *hlsl_get_type(struct hlsl_scope *scope, const char *name, bool recursive) 545 static const char * get_case_insensitive_typename(const char *name) 546 { 547 static const char *const names[] = 548 { 549 "dword", 550 }; 551 unsigned int i; 552 553 for (i = 0; i < ARRAY_SIZE(names); ++i) 554 if (!ascii_strcasecmp(names[i], name)) 555 { 556 return names[i]; 557 } changed this line in version 5 of the diff
I guess I didn't quite think the struct tests through; I forgot we use a separate rule to actually define struct types, and so the automated test doesn't prove anything one way or another. A manual test confirms that v4 is correct, though, at least.
In my defense, though, that error message is wrong and misled me; we can't be "redefining" anything in that rule. I'll send a patch to fix it.
Maybe it's worded like that because of the error message that Windows produces - "redefinition of type with struct/class". It's probably a side effect of something else happening in type name collision, obviously this can't define a new type. Using unique name will give another error right away.
Right now tests for eb02f7fd fail both on native and on vkd3d.
added 18 commits
-
ae7e5d41...5f904e50 - 15 commits from branch
wine:master
- 89121766 - vkd3d-shader/hlsl: Support case-insensitive lookup for builtin 'dword' type.
- df2d6d35 - vkd3d-shader/hlsl: Use unsigned type for the 'dword' alias.
- dd36215a - vkd3d-shader/hlsl: Support case-insensitive lookup for builtin 'float' type.
Toggle commit list-
ae7e5d41...5f904e50 - 15 commits from branch