-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pkg/compiler: 'fmt' fixes for baseless arguments #4924
base: master
Are you sure you want to change the base?
Conversation
if desc == typePtr && base.IsOptional { | ||
return // optional pointers prune recursion | ||
} | ||
for i, arg := range args { | ||
if desc.Args[i].Type == typeArgType { | ||
comp.recurseField(checked, arg, path) | ||
comp.recurseField(checked, arg, path, isArg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect that we need to update/reset isArg here.
There can be something like:
field ptr[in, fmt[hex, ...]]
then the top level is not fmt, but as we recurse we discover fmt.
Please add such test as well.
Theoretically in future there also can be something like fmt[hex, ptr[in, proc[0, 1, int32]]]
, where we start with fmt but as we recurse one level deep, we we again need base types.
I think isArg needs to be updated before each recurseField cal based on the current type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done as described, added tests as well.
pkg/compiler/types.go
Outdated
@@ -830,7 +830,7 @@ var typeFmt = &typeDesc{ | |||
Check: func(comp *compiler, t *ast.Type, args []*ast.Type, base prog.IntTypeCommon) { | |||
desc, _, _ := comp.getArgsBase(args[1], true) | |||
switch desc { | |||
case typeResource, typeInt, typeLen, typeFlags, typeProc: | |||
case typeResource, typeInt, typeLen, typeFlags, typeProc, typeConst: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this.
fmt[hex, const[0x1]]
looks like a strange way to create a fixed string, which is easier with stringnoz
type. This is especially suspicious given the current limitations of fmt type (fixed-size result).
In lots of cases when people try to do something like this, they are confused and don't really realize what they are doing.
I would prefer to wait for some real world use case for this first, and remove "const" from documentation instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I do not need this change, it was only there because of inconsistency between docs and code in the first place. I do not have a strong opinion on this matter. However, the reason I think it is better to have fmt+const is artificial, but it seems like a much cleaner way if you need constant in two different formats, i.e:
test.txt.const:
ID = 66
test.txt
data_1 {
name_with_dec_id stringnoz["somename"]
field_0 fmt[dec, const[ID]]
oz const[0x00]
}
data_2 {
name stringnoz["somename"]
id fmt[hex, const[ID]]
oz const[0x00]
}
But I understand the reasoning and added suggested change instead of mine. Considering the limitations, do you think it would be a good enhancement to have a format specifier for 'fmt'? It was somewhat beneficial for me to have that recently, but in my case it was much easier to patch the compiler itself in order to generate string with different size.
Fix for recurseField() pass that fails due to 'fmt' argument not having a type specifier, if used inside a structure.
Fixed 'fmt' documentation description at docs/syscall_descriptions_syntax.md.
New usecases for 'fmt' arguments after enabling 'proc' type in context of structures outlined in testdata/all.txt.
Pull request addresses two following issues:
More detailed explanation of the issues is here