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

Adding comments and const #788

Merged
merged 1 commit into from
Jun 5, 2019
Merged

Conversation

lains
Copy link
Contributor

@lains lains commented Jun 5, 2019

I open this PR because I have been browsing through the code, and it often took me a while tracing the meaning of function arguments, so I wrote some comments that I thought would be interesting to share to others as doxygen comments.

Also, and more importantly, I have been adding a few const type qualifiers to input arguments passed as pointers.
There are a lot of cases where structs are passed as pointers, most often only for input (they are unmodified).
Input and output arguments passed as reference can be explained by using doxygen @param[in] and @param[out] but I think a good and visual safeguard is also to qualify them as const so that the programmer and compiler both know this argument should not be modified.
Adding const qualification is a recursive action that can lead to changing many sub-function calls, so there is quite some work to be done, probably each time starting from higher level functions, and necessarily recursively reworking the prototypes of the sub-functions they use.

@lains lains added doc Related to documentation (source, Wiki, inline help etc...) cleanup labels Jun 5, 2019
Copy link
Member

@jkoan jkoan left a comment

Choose a reason for hiding this comment

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

Doc looks good

@lains lains added the WIP Work in progress. Do not merge as-is, but please contribute label Jun 5, 2019
@mvglasow mvglasow merged commit 9cf8520 into navit-gps:trunk Jun 5, 2019
@mvglasow
Copy link
Contributor

mvglasow commented Jun 5, 2019

Happy to see more Doxygen contributions. Thanks!

@pgrandin
Copy link
Contributor

pgrandin commented Jun 5, 2019

Nice work, thanks @lains !

@jkoan
Copy link
Member

jkoan commented Jun 6, 2019

Öhm guys, this PR had the WIP tag set

@lains
Copy link
Contributor Author

lains commented Jun 6, 2019

No worries, its was ready to merge. I just used the WIP tag because I wanted to add more of similar stuff, but I'll open new PRs for further work on this topic.

@mvglasow
Copy link
Contributor

mvglasow commented Jun 6, 2019

Sorry, I get trigger-happy when it comes to docs :-)

Personally, when I have docs to add, I commit them straight to trunk (after once being advised that for pure refactoring without even any code changes, there is no need to go through the whole review process).

@lains
Copy link
Contributor Author

lains commented Jun 7, 2019

OK, I'll do it this way (ie: go without review for added doc only). But I think I'll still go via a PR if there are const type qualifiers added as I'm interested in getting circle CI feedback... adding const may break compilation or at least add warnings and this might be in parts of the code that are platform-specific, so I may not have seen it when compiling on my machine.

@lains lains deleted the comments-and-const branch June 7, 2019 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup doc Related to documentation (source, Wiki, inline help etc...) WIP Work in progress. Do not merge as-is, but please contribute
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants