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

Enable C++ compilation #82

Closed
wants to merge 11 commits into from
Closed

Enable C++ compilation #82

wants to merge 11 commits into from

Conversation

Floessie
Copy link

Hi,

This PR renames a parameter that collides with a C++ keyword (new) and makes the library usable in C++ projects.

Best,
Flössie

@Floessie Floessie mentioned this pull request Mar 18, 2019
Copy link
Collaborator

@lsgunth lsgunth left a comment

Choose a reason for hiding this comment

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

The changes look good to me, but it could use a better description in the commit message.

@wesleywesley wesleywesley changed the base branch from master to devel March 19, 2019 01:47
@wesleywesley
Copy link
Contributor

@Floessie
Pls create your bug fix branch based on the latest devel branch.

Per our pull request process, we merge pull request into our devel branch first. I have change the target branch from microsemi:master to microsemi:devel.

And pls run "root/scripts/checkpatch.pl" for your patch, to make sure no error/warning exists.

Regard,
Wesley

@Floessie
Copy link
Author

Now I'm a bit puzzled. Maybe I should have read CONTRIBUTING.md earlier. 😉

Is this the right way to get this fix in?

  1. Drop this PR.
  2. Fork lsgunth/switchtec-user as per CONTRIBUTING.md.
  3. Branch off from devel (which doesn't exist in lsgunth/switchtec-user).
  4. Make changes.
  5. Run root/scripts/checkpatch.pl against git diff.
  6. Commit with a better commit message (is "Fix compilation with C++ compiler by replacing C++ keyword" better?).
  7. Create PR against lsgunth/switchtec-user.

@wesleywesley
Copy link
Contributor

  1. Drop this PR.
    -- It's ok.

  2. Fork lsgunth/switchtec-user as per CONTRIBUTING.md.

  3. Branch off from devel (which doesn't exist in lsgunth/switchtec-user).
    -- checkout -b "bug_fix_xxx" branch from microsemi/devel

  4. Make changes.

  5. Run root/scripts/checkpatch.pl against git diff.
    -- generate patch file by "git format-patch"
    -- run checkpatch.pl with the patch, fix the errors and warnings if required

  6. Commit with a better commit message (is "Fix compilation with C++ compiler by replacing C++ keyword" better?).
    -- Yes, this make sure you pass the checkpatch.pl check

  7. Create PR against lsgunth/switchtec-user.
    -- again microsemi/devel

@Floessie
Copy link
Author

@wesleywesley

  1. Drop this PR.
    -- It's ok.

One can't change the source branch of a PR on GitHub, so I'm forced to start a new PR and drop this one.

Thanks for the hints. I'll follow them in the new PR.

Maybe you should include this info in CONTRIBUTING.md.

@Floessie
Copy link
Author

@wesleywesley To which checkpatch.pl are you referring? Can't find it anywhere in the tree.

@wesleywesley
Copy link
Contributor

@Floessie
Copy link
Author

Using the checkpatch.pl you linked results in

Must be run from the top-level dir. of a kernel tree

Does this mean I have to checkout the kernel to provide you with a PR for a user space library?

@wesleywesley
Copy link
Contributor

@Floessie
If you agree, I will adopt your changes and create a new pull request, but follow the process we setup.

regard,
Wesley

@Floessie
Copy link
Author

@wesleywesley Sure, I'm fine with you adopting the changes. The scope of #80 was making you aware of a real concern: Using the library within a C++ project (there are reasons to use C++, e.g. GUI toolkits like Qt, Gtkmm, or wxWidgets) is currently impossible. The proposed patch was one way to fix that.

I was urged by Logan to turn the patch into a PR without being aware of the procedure to follow. Even if I had read CONTRIBUTING.md I would have got it wrong, because CONTRIBUTING.md doesn't describe the rules you had me to follow here. If you search for checkpatch within this repository this issue here is the only one mentioning it. If you want contributors to use checkpatch you should probably think about shipping a customized version of it with the project (others do that, too). And please, do correct CONTRIBUTING.md so that the next contributor won't look like a fool.

Thanks,
Flössie

@wesleywesley
Copy link
Contributor

@Floessie
Thx for your suggestion.

@wesleywesley
Copy link
Contributor

#85 is updated version of this patch, so close this one.

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.

None yet

3 participants