Skip to content
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

Codechange: Silence clang warnings about va_start #30

Merged
merged 1 commit into from
Dec 27, 2023

Conversation

glx22
Copy link
Contributor

@glx22 glx22 commented Dec 15, 2023

Fixes passing an object that undergoes default argument promotion to 'va_start' has undefined behavior [-Wvarargs]

rubidium42
rubidium42 previously approved these changes Dec 15, 2023
Copy link
Contributor

@rubidium42 rubidium42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I really like this construction.
On the other hand, the alternatives are much more invasive... although maybe consider swapping minSan and id in IssueMessage and something similar in CheckLength could help, but that means touching a lot of the code base, making this solution slightly more palatable from a review point of view.

@glx22
Copy link
Contributor Author

glx22 commented Dec 15, 2023

After reading the comment about swapping parameters I tried something in master...glx22:grfcodec:clang-warnings-2.
It's a rename of the variadic functions with swapping of parameters, and variadic templates with the original name and order calling the new variadic functions.

@rubidium42
Copy link
Contributor

... I tried something in master...glx22:grfcodec:clang-warnings-2...

I'm not fond of this approach because it's attempting to avoid the actual reorder, causing similar functions with different parameter order, which might cause further trouble later on. Though I can also imaging that it's going to cause a lot of churn given the number of IssueMessage calls.

@glx22
Copy link
Contributor Author

glx22 commented Dec 16, 2023

Yeah I count 15 CheckLength() calls, 465 IssueMessage() calls, and 3 vIssueMessage() calls.
That's a lot of changes, and for me swapping parameters also change the implicit understanding of what the functions are doing.
Like CheckLength(int alen,int elen,RenumMessageId message,...) is easily understood as compare alen and elen then show message (which also uses the remaining parameters) if they differ.
Or for IssueMessage(int minSan,RenumMessageId id,...) the first parameter showing the importance of the message, and id and the following parameters are the actual message.

So yeah, a global reorder doesn't seem the be a good idea.

@glx22
Copy link
Contributor Author

glx22 commented Dec 17, 2023

Now I have a third version in master...glx22:grfcodec:clang-warnings-3.
Main issue is I can't be sure it won't trigger assertions.

@rubidium42
Copy link
Contributor

I guess the -2 version is the least bad solution. I do not have any other ideas that would be relatively non-intrusive to solve these warnings.

@glx22
Copy link
Contributor Author

glx22 commented Dec 20, 2023

Hmm maybe a mix of this PR and -2, without RenumMessageId param in the XxxXxxI. So the entry point uses template (and requires RenumMessageId, then pass the args to internal (hiding the RenumMessageId for va_start) which then extract RenumMessageId param with va_args, then flow continues to vIssueMessage as before.

@glx22 glx22 merged commit c99d863 into OpenTTD:master Dec 27, 2023
8 checks passed
@glx22 glx22 deleted the clang-warnings branch December 27, 2023 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants