Fix GCC 7 errors (inc. buffer size and fix TString conversion) #17443
Conversation
TString supports conversions to std::string and std::string_view in C++17 which causes ambiguity as std::string ctors and operators support both also. Compiler cannot decide which one to use. We have either call explicitly conversion operatator on TString (e.g., via static_cast) or use other methods (e.g. construct std::string from const char * TString::Data()). In some cases compiler says that buffers are too small. TStrings could also be removed and replaced with C++ standard functions. Signed-off-by: David Abdurachmanov <David.Abdurachmanov@cern.ch>
|
A new Pull Request was created by @davidlt for CMSSW_9_0_X. It involves the following packages: Alignment/CocoaModel @ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @mmusich, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here #13028 |
|
please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
| @@ -128,7 +129,7 @@ OpticalAlignMeasurementInfo CocoaDaqReaderRoot::GetMeasFromPosition2D( AliDaqPos | |||
| OpticalAlignMeasurementInfo meas; | |||
|
|
|||
| meas.type_ = "SENSOR2D"; | |||
| meas.name_ = pos2D->GetID(); | |||
There was a problem hiding this comment.
@davidlt wouldn't it be sufficient to write this:
meas.name_ = pos2D->GetID().Data();
as the compiler knows how to convert a char to string?
Naively I would expect if we assign to a std::string the rhs will be implicitely converted to this type w/o going through another type. Or does the compiler optimize code such that it changes the type of meas.name_ from std::string to std::string_view?
There was a problem hiding this comment.
Implicit conversion is possible. Thus the following would work:
meas.name_ = pos2D->GetID().Data();
Which is basically the same as std::string(pos2D->GetID().Data()), because std::string has constructor which takes const char * and compiler can map that.
The problem is that TString has conversion functions to const char * and std::string_view, but the same are also available in std::string constructors. This means means we have two ways to constructor a string (or do things like +=) and they have the same priority. For compiler that's ambiguity -- it doesn't know which to pick. For example, the one below would force the compiler to pick const char * conversion operator of TString:
meas.name_ = static_cast<const char *>(pos2D->GetID());
As you can see there multiple ways you can write this. I depends how much you want to be explicit about what needs to happen or not, but end result (executed code) should be the same. One could write full chain explicitly instead of implicitly done by compiler:
meas.name_ = std::string(static_cast<const char *>(pos2D->GetID()));
| @@ -1090,7 +1090,7 @@ HIPAlignmentAlgorithm::calcAPE(double* par, int iter, double function) | |||
| void HIPAlignmentAlgorithm::bookRoot(void) | |||
| { | |||
| TString tname="T1"; | |||
| char iterString[5]; | |||
| char iterString[15]; | |||
| snprintf(iterString, sizeof(iterString), "%i",theIteration); | |||
There was a problem hiding this comment.
Same as previous, compiler error/notes tell the potential buffer size.
| @@ -837,7 +837,7 @@ void MuonAlignmentAnalyzer::endJob(){ | |||
|
|
|||
|
|
|||
|
|
|||
| char binLabel[15]; | |||
| char binLabel[40]; | |||
|
|
|||
There was a problem hiding this comment.
GCC error message provides information of min / max sizes for the buffer.
e.g,
Alignment/OfflineValidation/plugins/MuonAlignmentAnalyzer.cc:1371:25: note: 'snprintf' output between 18 and 38 bytes into a destination of size 15
|
+1 |
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
cmsbuild
added
alca-approved
fully-signed
and removed
alca-pending
pending-signatures
labels
|
+1 |
TString supports conversions to
std::stringandstd::string_viewinC++17 which causes ambiguity as
std::stringctors and operators supportboth also. Compiler cannot decide which one to use. We have either call
explicitly conversion operatator on
TString(e.g., viastatic_cast) oruse other methods (e.g. construct
std::stringfromconst char * TString::Data()).In some cases compiler says that buffers are too small.
TStrings could also be removed and replaced with C++ standard functions.Signed-off-by: David Abdurachmanov David.Abdurachmanov@cern.ch