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 warnings qt3support #662

Merged
merged 91 commits into from
Feb 19, 2017
Merged

Conversation

guitorri
Copy link
Member

@guitorri guitorri commented Feb 1, 2017

Closes #656.

@felix-salfelder
Copy link
Member

truly awesome. please merge (regardless of micro-regressions).

@guitorri
Copy link
Member Author

guitorri commented Feb 2, 2017

For some reason some deprecated constructor warning are not showing on my system (they show up on Windows and Linux builds).
Using -DQT_DEPRECATED_WARNINGS seems to do the trick. I will push more fixes shortly.

@guitorri guitorri force-pushed the fix-warnings-qt3support branch from 625d28a to c85be24 Compare February 7, 2017 21:45
@guitorri
Copy link
Member Author

guitorri commented Feb 7, 2017

Rebased, reverted the regression on pressing Esc. Silenced a few more warnings.
There are a few still warnings remaining. I will try to clear them tomorrow.

@guitorri
Copy link
Member Author

guitorri commented Feb 8, 2017

still a few things to fix

@guitorri
Copy link
Member Author

guitorri commented Feb 8, 2017

All warnings should be fixed. Shall we enable -Werror from now on?
I believe I tested everything. Please take a look.

@guitorri
Copy link
Member Author

Bump.
I think we should merge this before we start merging new features.

@in3otd
Copy link
Contributor

in3otd commented Feb 18, 2017

lemme do a last check 😁
About enabling -Werror I'm not sure but we could try and revert in case we don't like it...

@in3otd
Copy link
Contributor

in3otd commented Feb 18, 2017

...it seems clang++ on Travis (linux) does not like some Qt4 stuff and emits some warnings (?)

@guitorri
Copy link
Member Author

I am afraid there is not much we can do about these warnings. Qt4 is no longer supported on that version of OSX/Clang. I will try to have a look, if I find a fix I will let you know in another PR. 😄

@in3otd
Copy link
Contributor

in3otd commented Feb 18, 2017

I think I saw the warnings for the Qt4 in Linux actually... (but I do not see them here locally)

Instead, I just saw here I have also

graphictextdialog.cpp:34:67: warning: unused parameter 'name' [-Wunused-parameter]
savedialog.cpp:34:65: warning: unused parameter 'modal' [-Wunused-parameter]

besides a ton of clang-3.8: warning: -ldl: 'linker' input unused (as usual here), which is maybe just due to the not up-to-date `clang (?)

I'm starting to think that enabling -Werror is maybe too much...

@felix-salfelder
Copy link
Member

felix-salfelder commented Feb 18, 2017

-Werror is too much as a default build option (with or without debugging) simply because future compilers might warn about stuff that we cannot know yet.

yet, it might be pretty useful so set -Werror for CI, at least until we hit a warning that we absolutely do not agree with (or have no clue whatsoever). make CXXFLAGS=-Werror ...

@guitorri guitorri mentioned this pull request Feb 18, 2017
@guitorri
Copy link
Member Author

Right, we are not ready for -Werror. Even for CI it would create too much noise at this stage. Perhaps before a release we can make some sanitation until we see things are more or less under control.
I thing the solution to warning: -ldl: 'linker' input unused is to not pass the flag everywhere.
The warning: unused parameter are not Qt3Support issues. We should open another issue.

@in3otd in3otd merged commit 8ba9d11 into Qucs:develop Feb 19, 2017
@in3otd
Copy link
Contributor

in3otd commented Feb 19, 2017

yeah, as usual noticed a small issue just after merging: when opening a project I get a message on the console

Warning: QLayout: Attempting to add QLayout "" to SaveDialog "", which already has a layout

everything seems to work, anyway.

@guitorri guitorri deleted the fix-warnings-qt3support branch October 10, 2017 18:28
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.

4 participants