Skip to content

[MERGED] check all definitions of a function before suggesting 'const' #372

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

Closed
wants to merge 2 commits into from

Conversation

YashasSamaga
Copy link
Member

@YashasSamaga YashasSamaga commented Oct 3, 2018

What this PR does / why we need it:

const qualifier suggestions were being given based solely on the function definition that was currently being looked at. This caused wrong suggestions when one state of a function modifies the argument while another state did not.

The commit in this PR adds code to share usage details across multiple definitions of the functions. Hence, when the const qualifier is suggested, it takes into account of all the function states.

The commit is just a proof-of-concept for the time being. The commit only shares uWRITTEN status only but it could be generalized to share other usage flags as well which could solve #371 (comment)

Which issue(s) this PR fixes:

Fixes #371

What kind of pull this is:

  • A Bug Fix
  • A New Feature
  • Some repository meta (documentation, etc)
  • Other

Additional Documentation:

@YashasSamaga YashasSamaga requested a review from a team as a code owner October 3, 2018 11:30
@YashasSamaga YashasSamaga changed the base branch from master to dev October 3, 2018 11:30
@YashasSamaga
Copy link
Member Author

Ready.

@stale
Copy link

stale bot commented Jan 14, 2019

This issue has been automatically marked as stale because it has not had recent activity.

@Daniel-Cortez
Copy link
Contributor

This seems like a useful bugfix, as it saves from the need to manually apply #pragma unused as a workaround in all function states that don't modify the argument.
@YashasSamaga Why did you close this PR?

@YashasSamaga
Copy link
Member Author

YashasSamaga commented Jul 27, 2019

I lost touch with the code and I planned on writing a generic system for any flag instead of just uWRITTEN but never got time to do it. Also got annoyed with passing arguments to functions in this and the {...} PR. I think a better solution is required than having to pass int written everywhere or maybe I am thinking too much.

@Daniel-Cortez
Copy link
Contributor

Hmm... I see what you mean; a few functions in sc1.c indeed do have an unusually high number of arguments. But implementing a flag system might complicate the code even more. And even if it won't, does it really have to be done in this PR, and not in a separate one later (after this and the {...} PRs are merged)?

@YashasSamaga YashasSamaga reopened this Aug 2, 2019
@stale stale bot removed the state: stale label Aug 2, 2019
@Y-Less Y-Less closed this Sep 23, 2019
@Y-Less Y-Less mentioned this pull request Sep 24, 2019
@Y-Less Y-Less changed the title check all definitions of a function before suggesting 'const' [MERGED] check all definitions of a function before suggesting 'const' Sep 24, 2019
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.

4 participants