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

Preference for toggling preprocessor prototype generation #2324

Closed
wants to merge 1 commit into from
Closed

Preference for toggling preprocessor prototype generation #2324

wants to merge 1 commit into from

Conversation

JeffBobbo
Copy link

Added preproc.generate.prototypes preference setting to disable the generation of function prototypes (also done some cleanup) - Issues: #2304, #992 and #386 (workaround)

…eneration of function prototypes (also done some cleanup) - Issues: 2304, 992 and 386 (workaround)
@matthijskooijman
Copy link
Collaborator

Thanks, this is something we should indeed make optional. The code looks good to me.

Regarding the commit structure, you're really mixing up three changes in a single commit - removing prototypeCount, replacing List with ArrayList (why is this needed?) and adding the config entry. It would be better to split those out. Also, the commit message is not formatted according to git common practice: You should have a single short summary line (<= 70 characters preferably), then an empty line and then more details. Note that you can update this pullrequest by (force) pushing to your branch, no need to open a new one.

I'm wondering if it wouldn't be better to have a per-sketch preference for this, since globally enabling or disabling this probably is not the best approach (sketches will then come to rely on a specific setting, breaking if it's different). We don't currently have any plumbing in place for this yet, so this would require some further discussion (and having a global default still makes sense, even if we have a per-sketch setting)

@JeffBobbo
Copy link
Author

Never really used GitHub before (or git) so I mostly have no idea what I'm doing. :D

prototypeCount just wasn't needed (it was only used in writePrefix).
List -> ArrayList - I have no reason, it seemed like a good idea at the time.

@matthijskooijman
Copy link
Collaborator

Then it's probably best to drop the List -> ArrayList change, I think most of the code prefers using the interface type for variables instead of the implementation type (makes it easier to change the implementation later).

I'll try to talk you through updating your pullrequest. There's a few ways to go about this, but I propose you:

  • Make sure you have the branch with these changes checked out locally and have no uncommitted changes (check out git stash if you need to stash away some uncommitted changes for later).
  • run git reset HEAD^. This resets your branch's HEAD pointer to the parent (^) of the current HEAD; effectively this undoes the last commit. However, it does not touch the working copy, though, so your changes are still there.
  • run git add -p. This allows you to look at each chunks of the diff separately and decide wether to stage it or not. Select just the chunks belonging to the first change you want to commit.
  • run git commit. This makes a new commit using just the changes you previously staged.
  • Repeat the previous to steps for the other change.
  • You should now have just the ArrayList -> List change in your working copy (check with git diff). Run git checkout . to throw away all changes in the current directory of working copy (you reset all files to the state of the staging area, or if a file is not staged, the HEAD commit).
  • Run git push -f origin master (or something like that, depends a bit on how your remotes are setup) to push the new commits to github.

@JeffBobbo
Copy link
Author

Uhhh; I think I done something wrong cause this isn't looking quite right, not sure how to fix it either. =/

@matthijskooijman
Copy link
Collaborator

It seems nothing changed in the pullrequest, so I guess something went wrong at your end locally? Can't really help you without more info, though :-)

If you have IRC, feel free to poke me in #arduino on Freenode (I should be available for some time tonight or otherwise tomorrow).

@JeffBobbo
Copy link
Author

I've been idling there for the last couple weeks as Bobbo.

@ArduinoBot
Copy link
Contributor

Can one of the admins verify this patch?

@cmaglie cmaglie added feature request A request to make an enhancement (not a bug fix) Component: Compilation Related to compilation of Arduino sketches Component: IDE user interface The Arduino IDE's user interface labels Apr 15, 2015
@ffissore
Copy link
Contributor

Closing as wontfix. Preprocessing has much improved since the introduction of https://github.com/arduino/arduino-builder, available with the latest hourly build http://www.arduino.cc/en/Main/Software#hourly

@ffissore ffissore closed this Sep 29, 2015
@ffissore ffissore added the Type: Wontfix Arduino has decided that it will not resolve the reported issue or implement the requested feature label Sep 29, 2015
@ffissore ffissore added this to the Release 1.6.6 milestone Sep 29, 2015
@ffissore ffissore assigned ffissore and unassigned cmaglie Sep 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Compilation Related to compilation of Arduino sketches Component: IDE user interface The Arduino IDE's user interface feature request A request to make an enhancement (not a bug fix) Type: Wontfix Arduino has decided that it will not resolve the reported issue or implement the requested feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants