Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

Remove warns in clib.go #91

Closed
wants to merge 1 commit into from
Closed

Conversation

zapisanchez
Copy link
Contributor

C does not have an implicit conversion from const-qualified pointer types to non-const-qualified ones, so you should add an explicit one

@lestrrat
Copy link
Collaborator

@zapisanchez I'm sure what you are suggesting is correct, but a simple go test / build on my environment does not reproduce the warning. Can you describe why that is? I (belated) added a CI environment in GitHub Actions for this repository just now so you know what I do to check.

@zapisanchez
Copy link
Contributor Author

zapisanchez commented Nov 24, 2023

With a simple go test under arch linux this warns appears:

go test ./...

github.com/lestrrat-go/libxml2/clib
/home/zapi/go/pkg/mod/github.com/lestrrat-go/[email protected]/clib/clib.go: In function ‘MY_xmlLastError’:
/home/zapi/go/pkg/mod/github.com/lestrrat-go/[email protected]/clib/clib.go:61:16: warning: return discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
61 | return xmlGetLastError();
| ^~~~~~~~~~~~~~~~~

/home/zapi/go/pkg/mod/github.com/lestrrat-go/[email protected]/clib/clib.go: In function ‘MY_xmlCtxtLastError’:
/home/zapi/go/pkg/mod/github.com/lestrrat-go/[email protected]/clib/clib.go:67:16: warning: return discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
67 | return xmlCtxtGetLastError(ctx);
| ^~~~~~~~~~~~~~~~~~~~~~~~

I Bet this is for the same than last problem, my arch is using latest version of libxml-2
image

@lestrrat
Copy link
Collaborator

lestrrat commented Nov 24, 2023

Awwww, ubuntu doesn't have a deb for libxml2 2.12...
Can you please come up with a CI workflow to install the version you want, and then run go test. I know it's a minor task that I can do but I'm currently not all that motivated, so would appreciate it if you could come up with the repro in CI yourself

@zapisanchez
Copy link
Contributor Author

Hi @lestrrat I Have done a small refactor:

  • added go modules (go mod and o sum)
  • refactor due go mod
  • fix some lintings
  • Added actions it fails because we need this code merged in master of original branch in order to test)

I have tested in local and everything passed as usual
image

I know that's a big change.
I've not made changes on business logic
I've tried to make minor changes

It is not my intention to change your code, just to help you update it and clean it up. :D

lestrrat added a commit that referenced this pull request Nov 24, 2023
Copied from #91
@lestrrat
Copy link
Collaborator

@zapisanchez I'm very sorry, but I can't accept your PR as is. I don't claim that my code is better by any means, but I can't accept a PR that contains various different all jumbled into one :(

I have created #93 to apply your warnings fix and CI env. I haven't merged it in case you wanted credit for your work -- if you do, please create a PR with your commits that only contain what's on #93.

@zapisanchez
Copy link
Contributor Author

@lestrrat NP, you can merge #93 PR 👍

@zapisanchez
Copy link
Contributor Author

@lestrrat otherwise, i recommend update the code to use go modules and make linting cleanup

@lestrrat
Copy link
Collaborator

lestrrat commented Nov 24, 2023

@zapisanchez but I already did that in #92...? (which confused the hell out of me)

lestrrat added a commit that referenced this pull request Nov 24, 2023
* Add archlinux CI

Copied from #91

* See if this allows me to install a specific version

* fix version

* Apply casting to do away the const explicitly
@lestrrat
Copy link
Collaborator

closed by #93

@lestrrat lestrrat closed this Nov 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants