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.

Sign up for GitHub

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

Jump to bottom

Fix CMake and various warnings after merging #682 #694

Merged
merged 9 commits into from
Jul 20, 2017
Merged

Fix CMake and various warnings after merging #682 #694

merged 9 commits into from
Jul 20, 2017

Conversation

ra3xdh
Copy link
Contributor

@ra3xdh ra3xdh commented Jul 13, 2017 •
edited
Loading

This PR solves the following issues after merging #682:

  • CMake became broken. Added proper filenames in diagrams/CMakeLists.txt
  • There was a lot of deprecation warnings in diagramdialog.cpp on QComboBox::inserItem(). Replased by QComboBox::addItem().
  • Fixed "unused variable" i in diagramdialog.cpp. This variable was indeed unused.

There remains an unfixed warning in marker.cpp Line 388. It contains the following code:

if(n == diag()->nfreqt) n == diag()->nfreqt-1;

I didn't understand what this code should do. It seems, the comparison == should be replaced by assignment =. The current line does nothing, but all works as expected. @gildias, Could you please clarify, what exactly did you mean?

@ra3xdh ra3xdh self-assigned this Jul 13, 2017
@ra3xdh ra3xdh added this to the 0.0.20 milestone Jul 13, 2017
@ra3xdh
Copy link
Contributor Author

ra3xdh commented Jul 13, 2017 •
edited
Loading

marker.cpp Line 388.

I understood now what it should do. It checks the index limits. There should be an assignment.

@gildas
Copy link

gildas commented Jul 13, 2017

@ra3xdh, I think you got the wrong person.

@ra3xdh
Copy link
Contributor Author

ra3xdh commented Jul 13, 2017

@gildas, Sorry, I meant @gildias , an author of #682.

@gildas
Copy link

gildas commented Jul 13, 2017

Hey no problem.

@andresmmera
Copy link
Contributor

My fault. I didn't catch these issues.
I'll take a look to this PR as soon as I can.

@andresmmera andresmmera self-requested a review July 13, 2017 15:33
@andresmmera
Copy link
Contributor

@gildias and me detected some more warnings:

diagramdialog.cpp: In member function ‘void DiagramDialog::addvar(QString)’:
diagramdialog.cpp:1642:62: warning: logical not is only applied to the left hand side of comparison [-Wlogical-not-parentheses]
if( l != -1 && Var2.size() == (l + a.size()) && !m.size()>0)//Var2.size == (l + a.size()) in case of voltage (.v) to don't let pass a variable li
^
diagramdialog.cpp:1645:21: warning: ‘I’ may be used uninitialized in this function [-Wmaybe-uninitialized]
slotTakeVar(I);
^
graph.cpp: In member function ‘int Graph::getSelectedP(int, int)’:
graph.cpp:308:81: warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses]
if(((f1 >= f3) && (xn >= f3) && (xn <= f1)) || (f3 >= f1) && (xn >= f1) && (xn <= f3))

Working on that

@andresmmera
Firx for the warnings at graph.cpp and diagramdialog.cpp
ac43ddf 
This is a fix for the warnings arising at graph.cpp and
diagramdialog.cpp

In the case of the phasor diagrams, the graphs to be displayed are added
using Var2 instead of using the ChooseVars table. This means that it is
not necessary to pass a QTableWidgetItem object to 'slotTakeVar()' as
the graph selection is already managed there
@andresmmera
Copy link
Contributor

See ra3xdh#28

@andresmmera
Qucs filter: Fix for the warning related to the new features
ed76064 
Some warnings remained after merging Qucs#415. In this commit, two unused
variables are removed at qw_coupled_ring_filter.cpp. Moreover, the
substrate parameters for the capacitively coupled shunt resonators are
removed since there's no microstrip synthesis available
@andresmmera
Copy link
Contributor

See ra3xdh#29 too

ra3xdh added 2 commits July 15, 2017 15:52
@ra3xdh
Merge pull request #28 from andresmmera/cmake_fix_andresmmera
7f6aa54 
Fix for the warnings at graph.cpp and diagramdialog.cpp
@ra3xdh
Merge pull request #29 from andresmmera/Fix_Warnings_Recently_Added_F…
f087e98 
…eatures

Fix warnings recently added features
@ra3xdh
Copy link
Contributor Author

ra3xdh commented Jul 15, 2017

@andresmmera , I have just merged your PRs. Yes, warnings in qucs-filter could be merged too.

@andresmmera
Copy link
Contributor

I reviewed and tested this PR and I found no more issues so I believe it is ready to merge

@andresmmera andresmmera merged commit 2dace0c into Qucs:develop Jul 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andresmmera andresmmera Awaiting requested review from andresmmera

Assignees

@ra3xdh ra3xdh

Labels
easy-fix refactor
Projects
None yet
Milestone
0.0.20
Development

Successfully merging this pull request may close these issues.
3 participants
@ra3xdh @gildas @andresmmera