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

Combine OS X and Linux compiler flags. #15463

Merged
merged 8 commits into from
Mar 10, 2016
Merged

Combine OS X and Linux compiler flags. #15463

merged 8 commits into from
Mar 10, 2016

Conversation

quantumsteve
Copy link
Contributor

Description of work.

This simplifies the CMake script by setting flags for clang and gcc on OS X and Linux in a single location. I suppressed the remaining -Wcast-align warnings from Framework/ICat/src/GSoap/stdsoap2.cpp

I'm also sneaking in some fixes for -Winconsistent-missing-override warnings that are now in master.

To test:

  • Check that the OS X compiler flags are still being set (I suggest using the option CMAKE_EXPORT_COMPILER_COMMANDS and viewing the json file)
  • Verify that suppressing -Wcast-align warnings from Framework/ICat/src/GSoap/stdsoap2.cpp is acceptable.
  • Check that Xcode still picks up the correct flags, the C++14 standard and libc++ standard library.

Fixes #11681.

Does not need to be in the release notes.


Reviewer

Please comment on the following (full description):

Code Review
  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards? Is it well structured with small focussed classes/methods/functions?
  • Are there unit/system tests in place? Are the unit tests small and test the a class in isolation?
Functional Tests
  • Do changes function as described? Add comments below that describe the tests performed?
  • How do the changes handle unexpected situations, e.g. bad input?
  • Has the relevant documentation been added/updated?
  • Is user-facing documentation written in a user-friendly manner?
  • Has developer documentation been updated if required?
  • Does everything look good? Comment with the ship it emoji but don't merge. A member of @mantidproject/gatekeepers will take care of it.

@quantumsteve quantumsteve added this to the Release 3.7 milestone Feb 25, 2016
@quantumsteve
Copy link
Contributor Author

Below is another possible way to address the OS X warning. It builds and eliminates the reinterpret_cast and pointer arithmetic. I am, however, struggling to find a test to ensure it works correctly.

$ git diff
diff --git a/MantidPlot/src/MatrixModel.cpp b/MantidPlot/src/MatrixModel.cpp
index 8265d0c..65b2803 100644
--- a/MantidPlot/src/MatrixModel.cpp
+++ b/MantidPlot/src/MatrixModel.cpp
@@ -493,13 +493,13 @@ QImage MatrixModel::renderImage()
        d_matrix->range(&minValue, &maxValue);
     const QwtDoubleInterval intensityRange = QwtDoubleInterval (minValue, maxValue);
     for ( int i = 0; i < d_rows; i++ ){
-      QRgb *line = reinterpret_cast<QRgb *>(image.scanLine(i));
       for (int j = 0; j < d_cols; j++) {
-        double val = cell(i, j); // d_data[i*d_cols + j];
-        if (gsl_isnan(val))
-          *line++ = color_map.rgb(intensityRange, 0.0);
-        else if (fabs(val) < HUGE_VAL)
-          *line++ = color_map.rgb(intensityRange, val);
+        double val = cell(i, j);
+        if (gsl_isnan(val)){
+          image.setPixel(i,j,color_map.rgb(intensityRange, 0.0));
+        } else if (fabs(val) < HUGE_VAL){
+          image.setPixel(i,j,color_map.rgb(intensityRange, val));
+        }
       }
     }
     QApplication::restoreOverrideCursor();

@quantumsteve quantumsteve added the Maintenance Unassigned issues to be addressed in the next maintenance period. label Feb 27, 2016
@peterfpeterson peterfpeterson self-assigned this Mar 10, 2016
@peterfpeterson
Copy link
Member

The changes look reasonable. :shipit:

AndreiSavici added a commit that referenced this pull request Mar 10, 2016
Combine OS X and Linux compiler flags.
@AndreiSavici AndreiSavici merged commit 6f1cf28 into master Mar 10, 2016
@AndreiSavici AndreiSavici deleted the osx_warnings branch March 10, 2016 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Unassigned issues to be addressed in the next maintenance period.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix clang warnings
3 participants