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

Removed extra parameters from variadic functions' prototype in spuro.h #5

Merged
merged 1 commit into from
Apr 30, 2024
Merged

Removed extra parameters from variadic functions' prototype in spuro.h #5

merged 1 commit into from
Apr 30, 2024

Conversation

khushal-banks
Copy link
Contributor

Removed extra arguments from functions' prototypes declared in header file to fix #3

Tip

Please add clang compiler test in workflow

@jgabaut
Copy link
Owner

jgabaut commented Apr 29, 2024

I am confused.

Why did you open a new PR, closed the old one, and never replied to my questions? From my pov, I'm still waiting on them replies.

@khushal-banks
Copy link
Contributor Author

I am confused.

Why did you open a new PR, closed the old one, and never replied to my questions? From my pov, I'm still waiting on them replies.

I knew it .. i was wondering why you didn't review it and you forgot to publish your review.
None of your questions were visible to me.

Reason for discarding old PR is because i submitted one more commit which was breaking.
I will be adding new changes in a PR fashion from now on-wards so that i can revert them easily.

src/spuro.h Show resolved Hide resolved
src/spuro.h Show resolved Hide resolved
@jgabaut
Copy link
Owner

jgabaut commented Apr 29, 2024

That's fine, I just copied my comments over! :)

Can you see them now? @khushal-banks

Copy link
Contributor Author

@khushal-banks khushal-banks left a comment

Choose a reason for hiding this comment

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

It would be best if we can update makefile.yml to windows and macos platform and more tests for different compilers.

src/spuro.h Show resolved Hide resolved
src/spuro.h Show resolved Hide resolved
@jgabaut
Copy link
Owner

jgabaut commented Apr 30, 2024

I was thinking: this commit added a bunch of macro names to be called with no format (pass the plain cstring).

Would those be useless after this merge, if the changes solve the zero-arg conundrum?
Should the present names be refactored to point at new syntax?

@khushal-banks
Copy link
Contributor Author

I was thinking: this commit added a bunch of macro names to be called with no format (pass the plain cstring).

Would those be useless after this merge, if the changes solve the zero-arg conundrum? Should the present names be refactored to point at new syntax?

I missed them. Yes to all of your questions.
In fact, i should have used new macros names with modified definition (changes in this PR)

Can i add a commit to reflect those changes in this PR?

@jgabaut
Copy link
Owner

jgabaut commented Apr 30, 2024

In fact, i should have used new macros names with modified definition (changes in this PR)

Can i add a commit to reflect those changes in this PR?

I'm not sure what you mean.

Could you explain what you mean before, or add these changes to a different branch (on top of this one) and link to it, so that the PR can stay as-is for now?

@khushal-banks
Copy link
Contributor Author

khushal-banks commented Apr 30, 2024

I will see what i can do.

@jgabaut
Copy link
Owner

jgabaut commented Apr 30, 2024

I already tested on Linux and mingw32 and it seems all good, I can only further test macOS later in the day.

@jgabaut
Copy link
Owner

jgabaut commented Apr 30, 2024

In fact, i should have used new macros names with modified definition (changes in this PR)
Can i add a commit to reflect those changes in this PR?

I'm not sure what you mean.

Could you explain what you mean before, or add these changes to a different branch (on top of this one) and link to it, so that the PR can stay as-is for now?

Could you help me on this one? I'm trying to understand what you meant.

Do you mean something like still having a different name for functions forcing a format argument?

@khushal-banks
Copy link
Contributor Author

khushal-banks commented Apr 30, 2024

or add these changes to a different branch (on top of this one) and link to it, so that the PR can stay as-is for now?

Here is the git diff of new branch.
Here is the PR of new changes on top of this PR. I don't know how to add you there for review.

I am not sure but I think you want some of these changes but i am not sure which ones.

@jgabaut
Copy link
Owner

jgabaut commented Apr 30, 2024

I am not sure but I think you want some of these changes but i am not sure which ones.

Thanks a lot, this is really close to what I was thinking.

I'm gonna reply here if you don't mind, but don't close that one yet.

The question I'm asking myself is: think of printf().
It ends with a f, but it works with both explicit format and no format (plain cstring).
How does this relate to the mentioned clang warning? Is printf() using some kind of different trick to have correct behaviour for both cases, or is it just chalked up to the va_ logic like we would do in this PR? (From my tests, this seems to work as expected).

If it's the latter (it just relies on va_ functions like we would), then I think it's fine keeping the "f" in the names and just dropping the "string only" (no-format) macros would be the correct approach.

Still, I wonder if hiding this difference in the called macro would pose problems later. I guess as long as the underlying real signature has arguments before the variadic trail, we would be able to add new future parameters to the left part and old code could receive them by macro expansion with no problem.

Does keeping the explicit format argument in the macro signature allow for some selective logic that would not be possible otherwise? Any kind of validation being lost? I don't know.

Tested this PR and it seems to work as expected on aarch64-Linux, x86_64-Linux, x86_64-w64-ming32, arm64-darwin.

The real final question is, what advantages does the explicit format argument in macro definition allow?
I'm not sure if there are any.
Other than that, this seems a nice fix to the mentioned issue.

@khushal-banks
Copy link
Contributor Author

khushal-banks commented Apr 30, 2024

The question I'm asking myself is: think of printf().
It ends with a f, but it works with both explicit format and no format (plain cstring).
How does this relate to the mentioned clang warning? Is printf() using some kind of different trick to have correct behaviour for both cases,

Brother there is no trick.
Consider this,

// syntax for printf:
int printf(const char *restrict format, ...);

// syntax for our function:
void spr_logf_(const Spuro spr, SpuroLevel level, SpuroColor color, SpuroLoc loc, bool traced, bool timed, const char *format, ...);

They both are very similar. They both contain format argument. This clang issue is only with macros as explained below:

  • If we use format argument is macro definition then ... would have nothing so macro will end with a trailing ,.
  • On the other hand if we don't specify format argument in macro definition then ... would at least have format argument so no trailing ,

NOTE: syntax of spr_logf_ is not modified by PR.

If it's the latter (it just relies on va_ functions like we would), then I think it's fine keeping the "f" in the names and just dropping the "string only" (no-format) macros would be the correct approach.

I don't think keeping the "f" or removing it makes much difference. In my personal opinion, these macro would start making more sense if we remove "f" simply because spr_log is much more readable than spr_logf.

Still, I wonder if hiding this difference in the called macro would pose problems later. I guess as long as the underlying real signature has arguments before the variadic trail, we would be able to add new future parameters to the left part and old code could receive them by macro expansion with no problem.

Does keeping the explicit format argument in the macro signature allow for some selective logic that would not be possible otherwise? Any kind of validation being lost? I don't know.

For now, i don't see any issue. But i would say i am no expert. I happen to know solution for #3 so i tried my best to fix it. In my understanding compiler will first replace all macros with their expansion at pre-processing stage so this PR should pose no problem. Actually, it is only valid solution to solve #3 unless clang starts supporting ##

Tested this PR and it seems to work as expected on aarch64-Linux, x86_64-Linux, x86_64-w64-ming32, arm64-darwin.

The real final question is, what advantages does the explicit format argument in macro definition allow? I'm not sure if there are any. Other than that, this seems a nice fix to the mentioned issue.

I was wondering that there could be some unknown arch-specific implementation that could break this PR. If this works, please merge it. Because it won't be wrong. It should be acceptable to deal with unknown issues later.

For your final question. I would start my answer by saying feel free to ask more questions. I believe you are forgetting that macros are only text replacement. We are removing format argument from macro definition so it could take place of ... when no other arguments are specified.

@khushal-banks
Copy link
Contributor Author

I would request you to merge this PR.
Refactoring should be handled by a separate issue which would list:

  • names of macros that should contain f
  • names of macros that should not contain f
  • names of macros that should be removed
  • other changes (if necessary)

If you don't think this PR can be merged. I would happily close this.

@jgabaut jgabaut merged commit 4e1e47b into jgabaut:devel Apr 30, 2024
1 check passed
jgabaut added a commit that referenced this pull request Apr 30, 2024
* fix: include stdlib.h

* fix: Removed extra parameters from variadic functions' prototype in spuro.h (#5)

- Solve variadic macro warning for clang
- Drop usage of ## in macro definitions

Co-authored-by: Khushal <khushal.banks@gmail.com>
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