DM-10257: Fix compiler warnings in afw #250

kfindeisen · 2017-06-23T00:35:54Z

This PR eliminates all but one compiler warning (unused fits::{}::getFormatCode(int8_t)) when building afw with Ubuntu+GCC and MacOS+Clang. Justifications for individual changes are included in the commit messages.

During the MacOS port a few unrelated clang-format changes were made because of the mismatch in clang-format versions between Ubuntu and MacOS. @parejkoj and I allowed them to get committed on the grounds that we will probably standardize on clang-format 5 anyway.

Most of the warnings in GaussianProcess.cc were patched with on-the-spot casts. While changing the internal members to unsigned makes more sense a priori, the current code uses -1 as a null value, and it is very difficult to maintain KdTree's class invariants during the conversion. It may be less work to just replace the implementation of KdTree with an object-oriented rather than array-oriented implementation.

fits::{}::getFormatCode(int8_t) is unused, but has been left in because removing it would silently break code if a need for int8_t appeared later. Unfortunately, GCC's support for locally ignoring warnings is broken, so there's no way to mark the code as acceptable.

r-owen

Looks great. Two trivial suggestions

r-owen · 2017-06-23T16:03:58Z

src/table/io/FitsSchemaInputMapper.cc

+                {std::make_pair("PSF_FLUX", "slot_PsfFlux"), std::make_pair("AP_FLUX", "slot_ApFlux"),
+                 std::make_pair("INST_FLUX", "slot_InstFlux"), std::make_pair("MODEL_FLUX", "slot_ModelFlux"),
+                 std::make_pair("CALIB_FLUX", "slot_CalibFlux"), std::make_pair("CENTROID", "slot_Centroid"),
+                 std::make_pair("SHAPE", "slot_Shape")}};


Please run clang-format on this file. I found it results in minor but helpful improvements.

I thought the weird brace alignment was from clang-format 5... (@parejkoj can confirm)

Yes, that came from saving it in sublimetext with clang-format version 5.0.0 (tags/google/stable/2017-03-17). What version do you have, @r-owen ?

I agree the braces are weird. That's why I ran clang-format on the file (I guessed the braces were clang-format's fault and I wanted to check that). That confirmed by suspicions, but I was surprised when some other part of the file got reformatted (for the better, I think, though that's not very relevant).

So I suggest reformatting the whole file so all of it is in standard clang-format 5 format (ditto for any other C++ files you touched).

I am running 5.0.0 installed using Homebrew. Here is my config ~/.clang-format which I believe is LSST standard:

Language: Cpp BasedOnStyle: Google ColumnLimit: 110 IndentWidth: 4 AccessModifierOffset: -4 SortIncludes: false # reordering may break existing code ConstructorInitializerIndentWidth: 8 ContinuationIndentWidth: 8

My format file looks the same as yours, except with a --- as the first line, and ... as the last line (copied directly from the dev-guide).

I just re-ran clang-format outside of sublimeText, and it didn't change the file as checked-in. My version is also installed via homebrew.

Must be user error on my part. I just reformatted again and although Sublime Text claims the file changed, this time there are no visible changes (last time there were, and I'm not sure why).

r-owen · 2017-06-23T16:07:04Z

src/math/Interpolate.cc

@@ -157,7 +157,7 @@ ::gsl_interp_type const *styleToGslInterpType(Interpolate::Style const style) {
        case Interpolate::UNKNOWN:
            throw LSST_EXCEPT(pex::exceptions::InvalidParameterError,
                              "I am unable to make an interpolator of type UNKNOWN");
-        case Interpolate::NUM_STYLES:
+        default:


Adding a default clause is good, but removing the NUM_STYLES clause worries me. Some compilers warn if your case statements do not cover all cases (and I'm surprised clang did not).

I would expect a compiler to warn you if you have redundant cases in front of your default, but I guess I could add it back in and see what happens.

Ok, neither compiler cares. I'll leave it in.

I cares. If you use default the compiler can't check for missing enums. NUM_STYLES is probably there as it's in the enum and the compiler can/should whine if it's omitted.

My understanding is that ending with a default case is considered an important part of defensive programming, because even in C++ it is possible to provide illegal values (e.g. by casting).

I'm disappointed that compilers don't warn about missing values when a default clauses is included.

Without a default clause GCC gives a warning about falling through to the end of the function even when every constant is mentioned (despite also doing the check you mentioned).

I think I have an idea that will satisfy both rules, let me check.

I turns out recent gcc can warn in this situation, but it requires a flag: https://stackoverflow.com/questions/5402745/gcc-switch-on-enum-retain-missing-warning-but-use-default

What about:

case Interpolate::NUM_STYLES: throw LSST_EXCEPT(pex::exceptions::LogicError, str(boost::format("You can't get here: style == %") % style)); default: abort();

As a workaround, I've removed the default and added an exception after the switch. That lets GCC check that the cases are complete (and yes, I tested it) without complaining about reaching the end of the function. It's clunky, but I think it's the only way we can satisfy both constraints at the moment.

Russell's suggestion relies on #pragma GCC diagnostic, which is broken at the moment. It would certainly be my preferred solution otherwise. 😞

While the clang warning is a bug (shared_ptr::operator* is const, and therefore has no side effects), in most cases the use of typeid merely served to slow down the program. The code has been rearranged to minimize use of RTTI.

clang-format 5 on save

Remove fabs warnings for int and unsigned templates clang-format 5 on save

clang-format 5 on save

RobertLuptonTheGood · 2017-06-23T18:58:28Z

Does switching to a new-style enum solve this? They don't accept ints, and I don't think we ever OR these ones together. R P.S. the abort() is better than nowt, but I'd much prefer static testing.

kfindeisen · 2017-06-23T19:16:49Z

Good idea, but no, GCC still doesn't like the original function structure. (Also, Interpolate::Style is used as an integer quite frequently.)

The new current version (as of 30 minutes ago) allows static testing, so I propose we live with it.

PaulPrice · 2017-06-23T19:34:59Z

src/math/Interpolate.cc

@@ -161,6 +161,9 @@ ::gsl_interp_type const *styleToGslInterpType(Interpolate::Style const style) {
            throw LSST_EXCEPT(pex::exceptions::LogicError,
                              str(boost::format("You can't get here: style == %") % style));
    }
+    // don't use default statement to let compiler check switch coverage
+    throw LSST_EXCEPT(pex::exceptions::LogicError,
+                      str(boost::format("You can't get here: style == %") % style));


If you can't ever get there, this is a programming error, and therefore an abort() is more appropriate than an exception.

An abort would cause the entire Stack to crash, with no explanation, in the middle of processing. It certainly wouldn't make it obvious that the programmer forgot to include an enum.

There is simply no justification for calling abort() in low-level functions in modern code.

That is exactly the point! You want the entire stack to crash (with a core dump) because you're in a completely bad situation that cannot be recovered from. If you throw an exception it might be caught and ignored, which makes it so much harder to know there's a problem at all, let alone debug it.

I will say that I've had this same argument with folks before. I'm firmly on @kfindeisen's side -- I want a proper stack trace when something goes wrong, so I always thrown an exception. But there are certainly those in the project who are prefer C++ assert for this situation. I don't think it'll get resolved at our level, and rather than bring in the big guns for such a small and helpful ticket, I suggest we keep what @kfindeisen has done and move on.

...If for no other reason than because it is the minimal change from what we had already!

You can get "a proper stack trace" from the core dump, or from re-running under a debugger. Throwing an exception does not guarantee that you're going to get a stack trace at all, or ever even notice that there's something horribly wrong.

It's absolutely essential when we know that the system is in an inconsistent state (e.g., smashed stack, memory corruption) that we not attempt to push forward and make things worse and undebuggable.

Agreed for memory corruption. That is not the concern here. A simple cast can get you this issue.

I think it's worth doing even for "a simple cast" error, but I give up.

kfindeisen added 2 commits June 22, 2017 10:33

kfindeisen requested a review from r-owen June 23, 2017 00:35

r-owen approved these changes Jun 23, 2017

View reviewed changes

kfindeisen force-pushed the tickets/DM-10257 branch from 001f7f3 to c78bf9d Compare June 23, 2017 18:14

kfindeisen and others added 6 commits June 23, 2017 11:44

Fix no-return warning.

70d204f

Fix clang warnings for typeid(*shared_ptr).

90fd9c1

While the clang warning is a bug (shared_ptr::operator* is const, and therefore has no side effects), in most cases the use of typeid merely served to slow down the program. The code has been rearranged to minimize use of RTTI.

Remove unused typedefs

0188be2

clang-format 5 on save

Use double arithmetic for all image types

1981cd3

Remove fabs warnings for int and unsigned templates clang-format 5 on save

Remove unused nanAngle

8655643

clang-format 5 on save

Include optional braces to make clang happy

9668c66

clang-format 5 on save

kfindeisen force-pushed the tickets/DM-10257 branch from c78bf9d to 9668c66 Compare June 23, 2017 18:45

PaulPrice reviewed Jun 23, 2017

View reviewed changes

kfindeisen merged commit 9668c66 into master Jun 23, 2017

kfindeisen deleted the tickets/DM-10257 branch August 22, 2018 19:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-10257: Fix compiler warnings in afw #250

DM-10257: Fix compiler warnings in afw #250

kfindeisen commented Jun 23, 2017

r-owen left a comment

r-owen Jun 23, 2017

kfindeisen Jun 23, 2017

parejkoj Jun 23, 2017

r-owen Jun 23, 2017 •

edited

parejkoj Jun 23, 2017

r-owen Jun 23, 2017

r-owen Jun 23, 2017

kfindeisen Jun 23, 2017 •

edited

kfindeisen Jun 23, 2017

RobertLuptonTheGood Jun 23, 2017 •

edited

r-owen Jun 23, 2017 •

edited

kfindeisen Jun 23, 2017

r-owen Jun 23, 2017

PaulPrice Jun 23, 2017

kfindeisen Jun 23, 2017

kfindeisen Jun 23, 2017

RobertLuptonTheGood commented Jun 23, 2017 via email

kfindeisen commented Jun 23, 2017

PaulPrice Jun 23, 2017

kfindeisen Jun 23, 2017 •

edited

PaulPrice Jun 23, 2017

r-owen Jun 23, 2017

r-owen Jun 23, 2017

PaulPrice Jun 23, 2017

r-owen Jun 23, 2017

PaulPrice Jun 23, 2017

DM-10257: Fix compiler warnings in afw #250

DM-10257: Fix compiler warnings in afw #250

Conversation

kfindeisen commented Jun 23, 2017

r-owen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Jun 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen Jun 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobertLuptonTheGood Jun 23, 2017 • edited

Choose a reason for hiding this comment

r-owen Jun 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobertLuptonTheGood commented Jun 23, 2017 via email

kfindeisen commented Jun 23, 2017

Choose a reason for hiding this comment

kfindeisen Jun 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Jun 23, 2017 •

edited

kfindeisen Jun 23, 2017 •

edited

RobertLuptonTheGood Jun 23, 2017 •

edited

r-owen Jun 23, 2017 •

edited

kfindeisen Jun 23, 2017 •

edited