Skip to content

add #line directive to additional source files #325

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 1 commit into from

Conversation

slady
Copy link

@slady slady commented Jun 20, 2019

improvement for #323

Copy link
Collaborator

@matthijskooijman matthijskooijman left a comment

Choose a reason for hiding this comment

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

This looks like a sane approach, so I'm in favour of merging this.

Code looks good to me. I haven't tested the code, though.

Two doubts I have:

  • For what kinds of files is this #line directive now added? .cpp, .c and .h files are of course fine, but how about assembly files? Can there be any other types of files in the list of "Additional files" that do not like #line directives?
  • When files to be modified are really big, then the in-memory prepending of the line directive might be ineffecient (compared to writing the line directive first and then the original contents). I can see why we need this to compare the file contents, though. Also, source files are usually not really big and since we're reading things into memory in the first place, that's already inefficient and suited for big (e.g. 10's or 100's of MB) files anyway, so I guess this is a non-issue.

@slady
Copy link
Author

slady commented Jun 20, 2019

To your first doubt:
I double checked the contents of the additional files list, it can contain files with these extensions:
.h .c .hpp .hh .cpp .s
Hope all of them can start with the #line directive.

To your second doubt:
Reading the whole file into memory seems a little bit odd and superfluous, but it was already in the source code long before my changes took place. My change only added one little line to the contents of the whole file. Hope this did not blow it up too much.

@oschonrock
Copy link

oschonrock commented May 14, 2020

+1 This would be super useful.

I presume this would "automatically" be part of ardino-cli ? Which is what I am using, and what people who care about correct error messages could be likely to use?

Any way to move this along? Now has conflicts?

@slady
Copy link
Author

slady commented May 14, 2020

the bad news is they had changed the master, without incorporating my changes, so now we have conflicts... :-(

the good news is that my friend already rewrote my code to the new base :-)

we can try to re-apply his changes, but I highly doubt those would get merged into the main base... :-/

so our only hope is to fork the project and take control over it! ;-)

@oschonrock
Copy link

Shame, it's a pretty trivial change, which is not very invasive and very useful .

Is there a pull request for the updated version against current master?

@matthijskooijman
Copy link
Collaborator

I believe this code was moved to the arduino-cli repo, so any new PR should be created there. There's also quite some movement in that repo, so I'm hopeful it can be merged in there.

@slady
Copy link
Author

slady commented May 14, 2020

I am not active on arduino any more, since I am writing a game for 8-bit computer ZX Spectrum right now along with a new IO game online game

however I will let my friend know that you are interested in this kind of change so he can step in and take over the hassle of communication with the arduino development team

@oschonrock
Copy link

I am running arduino-cli V 0.10.0, which includes this line:

https://github.com/arduino/arduino-cli/blob/ec5c3ed105b32c5654fd60131a667f8557b196d5/arduino/builder/sketch.go#L242

Which looks to me like it should be doing what we want.

So this actually becomes a different case altogether? ie, a bug/issue, why is it not working on my code. I will investigate. Could well be to do with arduino-cli expecting my "extra files" to have a different extension or something like that.

Anyway, I think we should close this issue, it can never be merged. And "in spirit" at least, it has been included in arduino-cli.

Agree?

@matthijskooijman
Copy link
Collaborator

That line applies to the merged .ino files (e.g. the MainFile and the OtherSketchFiles, which together are all .ino files). This issue is about the AdditionalFiles (maybe not so brilliant terminology when you look at it like this :-p), which are copied verbatim a few lines further down.

@oschonrock
Copy link

@matthijskooijman Yeah, I just kind of figured that out too. Arduino does do some really weird preprocessing, coming from a CPP background.

We just need a single line inserted at the beginning of each "additional file" copy. So this should not be too hard. I know no Go at all, but I think even I can manage that with a bit of "copy/paste coding".

Will have a "Go"... ;-) and make a pull request.

Thanks

@oschonrock
Copy link

It was a one-liner. Here is the pull request: arduino/arduino-cli#707
Hope it gets through

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@per1234
Copy link
Contributor

per1234 commented Sep 30, 2021

Closing as superseded by arduino/arduino-cli#1224. Thanks everyone for your contributions!

@per1234 per1234 closed this Sep 30, 2021
@per1234 per1234 added conclusion: duplicate Has already been submitted topic: code Related to content of the project itself type: enhancement Proposed improvement labels Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conclusion: duplicate Has already been submitted topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants