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

Fix:core:Fix item_def.h when used whithout defined macros (like in IDE) #771

Merged
merged 3 commits into from
Jun 5, 2019

Conversation

jkoan
Copy link
Member

@jkoan jkoan commented Apr 4, 2019

Hi,

always when in a IDE like Eclipse i head the problem that all attr_* definitions where not found by the IDE and therefore where marked as error, so my approach now is to declare them as extern so the IDE does not get annoyed by this. This also fixes auto competition.

@mvglasow
Copy link
Contributor

mvglasow commented Apr 4, 2019

Much needed indeed (this has been bothering me as well for some time :-)

The only doubt I have is related to this comment:

/* This file is generated from http://wiki.navit-project.org/index.php/Item_def.h, do not edit it, edit the wiki page instead */

Does that still reflect the current status? If yes, we’d need to make sure the extra declarations survive an update. If not, the comment should be removed.

@jkoan
Copy link
Member Author

jkoan commented Apr 4, 2019

I think this is not up to date. At least we don't do this automatically. There is a script for that but I don't even know if it is functional. So my opinion would be to remove the comment

@mvglasow
Copy link
Contributor

mvglasow commented Apr 6, 2019

I am generally not convinced that generating a source file from a wiki page is such a great idea. However, I would suggest doing a test run of the script to see if it runs at all.

  • If the script fails and is not trivial to fix, I’d suggest removing it. If the fix is easy, fix and retry.
  • If the script runs but ends up removing item types (suggesting the wiki is out of date), I’d also suggest removing the script.
  • If the script runs and generates essentially the same file (without the fixes), then I would suggest extending the script to generate item_def.h with the fixes.

With that I would give my approval. If we end up removing the script, we might consider updating the wiki from the source file—but that doesn’t have to go in this pull request.

BTW, Eclipse equally nags about stuff in attr_def.h—would the fix work there as well?

@pgrandin
Copy link
Contributor

100% onboard with @mvglasow 's approach.

@jkoan
Copy link
Member Author

jkoan commented May 17, 2019

I am generally not convinced that generating a source file from a wiki page is such a great idea. However, I would suggest doing a test run of the script to see if it runs at all.

* If the script fails and is not trivial to fix, I’d suggest removing it. If the fix is easy, fix and retry.

* If the script runs but ends up removing item types (suggesting the wiki is out of date), I’d also suggest removing the script.

* If the script runs and generates essentially the same file (without the fixes), then I would suggest extending the script to generate `item_def.h` with the fixes.

With that I would give my approval. If we end up removing the script, we might consider updating the wiki from the source file—but that doesn’t have to go in this pull request.

The Script is functional and works but the wiki is outdated. So remove Script and edit wiki to say that item_defs are now handled in the item_def.h file?

Currently the state of the Wiki would breake things as Items would be added what would change places.

BTW, Eclipse equally nags about stuff in attr_def.h—would the fix work there as well?
We Could fix this as well, i will open another PR for this

@mvglasow
Copy link
Contributor

The Script is functional and works but the wiki is outdated. So remove Script and edit wiki to say that item_defs are now handled in the item_def.h file?

Sounds good to me—and remove the comment in item_def.h, which is now obsolete.

@jkoan
Copy link
Member Author

jkoan commented May 17, 2019

I will do this in another pr where I also remove the script

@mvglasow mvglasow merged commit d2f3512 into trunk Jun 5, 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.

3 participants