Compiler warnings

[tvami]

Describe the bug
In the idea of having clean compiler logs, here are a few more warnings I see:

Type "control reaches end of non-void function"

/Users/tav/Documents/1Research/LDMX/ldmx-sw/DetDescr/src/DetDescr/HcalGeometry.cxx: In member function 'std::vector<double> ldmx::HcalGeometry::rotateGlobalToLocalBarPosition(const std::vector<double>&, const ldmx::HcalID&) const':
/Users/tav/Documents/1Research/LDMX/ldmx-sw/DetDescr/src/DetDescr/HcalGeometry.cxx:77:1: warning: control reaches end of non-void function [-Wreturn-type]
   77 | }
      | ^
/Users/tav/Documents/1Research/LDMX/ldmx-sw/DetDescr/src/DetDescr/HcalGeometry.cxx: In member function 'ldmx::HcalGeometry::ScintillatorOrientation ldmx::HcalGeometry::getScintillatorOrientation(ldmx::HcalID) const':
/Users/tav/Documents/1Research/LDMX/ldmx-sw/DetDescr/src/DetDescr/HcalGeometry.cxx:128:1: warning: control reaches end of non-void function [-Wreturn-type]
  128 | }
      | ^

Type " declaration of 'm' shadows a global declaration "

/Users/tav/Documents/1Research/LDMX/ldmx-sw/SimCore/G4DarkBreM/app/simulate.cxx: In constructor 'g4db::example::Hunk::Hunk(double, const string&)':
/Users/tav/Documents/1Research/LDMX/ldmx-sw/SimCore/G4DarkBreM/app/simulate.cxx:121:37: warning: declaration of 'm' shadows a global declaration [-Wshadow]
  121 |   Hunk(double d, const std::string& m)
      |                  ~~~~~~~~~~~~~~~~~~~^
In file included from /usr/local/include/Geant4/CLHEP/Units/PhysicalConstants.h:42,
                 from /usr/local/include/Geant4/G4ParticleDefinition.hh:58,
                 from /usr/local/include/Geant4/G4ParticleTable.hh:58,
                 from /usr/local/include/Geant4/G4VUserPhysicsList.hh:92,
                 from /usr/local/include/Geant4/G4VModularPhysicsList.hh:59,
                 from /usr/local/include/Geant4/QBBC.hh:42,
                 from /Users/tav/Documents/1Research/LDMX/ldmx-sw/SimCore/G4DarkBreM/app/simulate.cxx:11:
/usr/local/include/Geant4/CLHEP/Units/SystemOfUnits.h:108:23: note: shadowed declaration is here
  108 |   static const double m  = meter;
      |                       ^

Type "deprecated"

/Users/tav/Documents/1Research/LDMX/ldmx-sw/Tools/src/Tools/ONNXRuntime.cxx: In constructor 'ldmx::Ort::ONNXRuntime::ONNXRuntime(const string&, const Ort::SessionOptions*)':
/Users/tav/Documents/1Research/LDMX/ldmx-sw/Tools/src/Tools/ONNXRuntime.cxx:70:30: warning: 'void Ort::detail::TensorTypeAndShapeInfoImpl<T>::GetDimensions(int64_t*, size_t) const [with T = Ort::detail::Unowned<const OrtTensorTypeAndShapeInfo>; int64_t = long int; size_t = long unsigned int]' is deprecated: use GetShape() [-Wdeprecated-declarations]
   70 |     tensor_info.GetDimensions(input_node_dims_[input_name].data(), num_dims);
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /Users/tav/Documents/1Research/LDMX/ldmx-sw/Tools/include/Tools/ONNXRuntime.h:10,
                 from /Users/tav/Documents/1Research/LDMX/ldmx-sw/Tools/src/Tools/ONNXRuntime.cxx:2:
/usr/local/include/onnxruntime_cxx_api.h:858:41: note: declared here
  858 |   [[deprecated("use GetShape()")]] void GetDimensions(int64_t* values, size_t values_count) const;  ///< Wraps OrtApi::GetDimensions
      |                                         ^~~~~~~~~~~~~
/Users/tav/Documents/1Research/LDMX/ldmx-sw/Tools/src/Tools/ONNXRuntime.cxx:92:30: warning: 'void Ort::detail::TensorTypeAndShapeInfoImpl<T>::GetDimensions(int64_t*, size_t) const [with T = Ort::detail::Unowned<const OrtTensorTypeAndShapeInfo>; int64_t = long int; size_t = long unsigned int]' is deprecated: use GetShape() [-Wdeprecated-declarations]
   92 |     tensor_info.GetDimensions(output_node_dims_[output_name].data(), num_dims);
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/local/include/onnxruntime_cxx_api.h:2088,
                 from /Users/tav/Documents/1Research/LDMX/ldmx-sw/Tools/include/Tools/ONNXRuntime.h:10,
                 from /Users/tav/Documents/1Research/LDMX/ldmx-sw/Tools/src/Tools/ONNXRuntime.cxx:2:
/usr/local/include/onnxruntime_cxx_inline.h:1061:13: note: declared here
 1061 | inline void TensorTypeAndShapeInfoImpl<T>::GetDimensions(int64_t* values, size_t values_count) const {
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Boost deprecation

In file included from /usr/include/boost/smart_ptr/detail/sp_thread_sleep.hpp:22,
                 from /usr/include/boost/smart_ptr/detail/yield_k.hpp:23,
                 from /usr/include/boost/smart_ptr/detail/spinlock_gcc_atomic.hpp:14,
                 from /usr/include/boost/smart_ptr/detail/spinlock.hpp:42,
                 from /usr/include/boost/smart_ptr/detail/spinlock_pool.hpp:25,
                 from /usr/include/boost/smart_ptr/shared_ptr.hpp:29,
                 from /usr/include/boost/shared_ptr.hpp:17,
                 from /usr/include/boost/python/converter/shared_ptr_to_python.hpp:12,
                 from /usr/include/boost/python/converter/arg_to_python.hpp:15,
                 from /usr/include/boost/python/call.hpp:15,
                 from /usr/include/boost/python/object_core.hpp:14,
                 from /usr/include/boost/python/args.hpp:22,
                 from /usr/include/boost/python.hpp:11,
                 from /Users/tav/Documents/1Research/LDMX/BugFixes/ldmx-sw/DetDescr/src/DetDescr/DetectorIDBindings.cxx:2:
/usr/include/boost/bind.hpp:36:1: note: '#pragma message: The practice of declaring the Bind placeholders (_1, _2, ...) in the global namespace is deprecated. Please use <boost/bind/bind.hpp> + using namespace boost::placeholders, or define BOOST_BIND_GLOBAL_PLACEHOLDERS to retain the current behavior.'
   36 | BOOST_PRAGMA_MESSAGE(
      | ^~~~~~~~~~~~~~~~~~~~
/usr/include/boost/detail/iterator.hpp:13:1: note: '#pragma message: This header is deprecated. Use <iterator> instead.'
   13 | BOOST_HEADER_DEPRECATED("<iterator>")
      | ^~~~~~~~~~~~~~~~~~~~~~~

and

  /home/runner/work/ldmx-sw/ldmx-sw/Framework/test/ConfigurePythonTest.cxx: In function 'void CATCH2_INTERNAL_TEST_0()':
  /home/runner/work/ldmx-sw/ldmx-sw/Framework/test/ConfigurePythonTest.cxx:166:15: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings]
    166 |     args[0] = "9000";

Additional context

Similar issue at #1199

[tvami]

• For the first one, should we just return empty vector (return {}; outside the switch statements?
• For the 2nd, hmm maybe better to avoid one letter variable names?
• For the 3rrd, I guess we should use GetShape(), but I have no idea if it's a one to one translation (meaning, will the same arguments be needed)?

[EinarElen]

Thanks for making this issue! Cleaning up our builds and allowing us to start making use of more static analysis as part of the CI is an important goal. There's a overarching issue for these things in #1176. I'll make a list of related issues in there and link this one

Since LDMX-Software/docker#67 and LDMX-Software/cmake#19 we have some additional tools in the development container that can give additional static analysis. For compiler warnings, there are some CMake flags you can enable to add additional warnings that you can see in https://github.com/LDMX-Software/cmake/blob/trunk/CompilerWarnings.cmake

You can also build ldmx-sw with the clang++ compiler instead of GCC. There will be some warnings that clang catches that GCC doesn't and vice versa and sometimes the different compilers give better/worse feedback (see e.g. the warning in LDMX-Software/Hcal#70 and LDMX-Software/Framework#70).

Finally, for memory-issues we also have #1172 so if you find anything in that realm it might be discussed in there already.

w.r.t. the first one here, I think reaching the end of these functions should clearly signal a bug. From looking at the code, it isn't immediately obvious to me if there is a way to reach the end of those functions with valid input so I would probably suggest raising an exception.

[EinarElen]

I should say that it isn't obvious to me there's a way to trigger the end of those functions doesn't mean that the warning is silly, there have been similar problems caught by this warning before
ebbe456

[EinarElen]

For building with clang, you may have to manually merge LDMX-Software/TrigScint#54 first. This reminded me that we should probably get around to merging it anyways.

[tomeichlersmith]

The second one is in the G4DarkBreM repo which I can patch pretty quickly.

[tomeichlersmith]

For the 3rd...

I also am unsure if GetShape is just a drop-in replacement, but there is an additional complexity. The v3 development container images have ONNX v1.2 which does not have GetShape while the v4 images have ONNX v1.15 which issues this deprecation warning. The good news is that we pin the ONNX version so we can pretty safely assume that GetDimension won't disppear on us despite this deprecation warning. The bad news is that if we want to remove this deprecation warning, we'd need to either completely drop v3 images (not something I'm willing to do) or do some ugly preprocessor stuff to align the APIs like I do with getting names:

ldmx-sw/Tools/src/Tools/ONNXRuntime.cxx

Lines 15 to 37 in ddcc4da

	#if ORT_API_VERSION == 2
	// version used when first integrated onnx into ldmx-sw
	// and version downloaded by cmake infrastructure
	// only support x86_64 architectures
	std::string get_input_name(std::unique_ptr<Session>& s, size_t i, AllocatorWithDefaultOptions a) {
	return s->GetInputName(i, a);
	}
	std::string get_output_name(std::unique_ptr<Session>& s, size_t i, AllocatorWithDefaultOptions a) {
	return s->GetOutputName(i, a);
	}
	#else
	// latest version with prebuilds for both x86_64 and arm64
	// architectures but contains a slight API change
	std::string get_input_name(std::unique_ptr<Session>& s, size_t i, AllocatorWithDefaultOptions a) {
	return s->GetInputNameAllocated(i, a).get();
	}
	std::string get_output_name(std::unique_ptr<Session>& s, size_t i, AllocatorWithDefaultOptions a) {
	return s->GetOutputNameAllocated(i, a).get();
	}
	#if ORT_API_VERSION != 15
	#pragma warning ("Untested ONNX version, not certain of API, assuming API version 15.")
	#endif
	#endif

[EinarElen]

The other option is to disable the warning, but I would prefer doing some #pragma's if it isnt happening in a lot of places

https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html

With some comment referencing this issue of course

[tvami]

hey @tomeichlersmith your point in #1208 (comment) is now relevant again since you did drop v3 images recently, right?

[tvami]

So besides ONNX warning, I see this one

In file included from /usr/include/boost/smart_ptr/detail/sp_thread_sleep.hpp:22,
                 from /usr/include/boost/smart_ptr/detail/yield_k.hpp:23,
                 from /usr/include/boost/smart_ptr/detail/spinlock_gcc_atomic.hpp:14,
                 from /usr/include/boost/smart_ptr/detail/spinlock.hpp:42,
                 from /usr/include/boost/smart_ptr/detail/spinlock_pool.hpp:25,
                 from /usr/include/boost/smart_ptr/shared_ptr.hpp:29,
                 from /usr/include/boost/shared_ptr.hpp:17,
                 from /usr/include/boost/python/converter/shared_ptr_to_python.hpp:12,
                 from /usr/include/boost/python/converter/arg_to_python.hpp:15,
                 from /usr/include/boost/python/call.hpp:15,
                 from /usr/include/boost/python/object_core.hpp:14,
                 from /usr/include/boost/python/args.hpp:22,
                 from /usr/include/boost/python.hpp:11,
                 from /Users/tav/Documents/1Research/LDMX/BugFixes/ldmx-sw/DetDescr/src/DetDescr/DetectorIDBindings.cxx:2:
/usr/include/boost/bind.hpp:36:1: note: '#pragma message: The practice of declaring the Bind placeholders (_1, _2, ...) in the global namespace is deprecated. Please use <boost/bind/bind.hpp> + using namespace boost::placeholders, or define BOOST_BIND_GLOBAL_PLACEHOLDERS to retain the current behavior.'
   36 | BOOST_PRAGMA_MESSAGE(
      | ^~~~~~~~~~~~~~~~~~~~
/usr/include/boost/detail/iterator.hpp:13:1: note: '#pragma message: This header is deprecated. Use <iterator> instead.'
   13 | BOOST_HEADER_DEPRECATED("<iterator>")
      | ^~~~~~~~~~~~~~~~~~~~~~~

I dont know if I can do anything with this, @tomeichlersmith @EinarElen ?

[EinarElen]

I would probably just add the #define that is referenced and add a comment in the docker repo by the dependencies to check this whenever we update boost