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 GCC 7 errors (inc. buffer size and fix TString conversion) #17443

Merged
merged 1 commit into from Feb 10, 2017
Merged

Fix GCC 7 errors (inc. buffer size and fix TString conversion) #17443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 5 additions & 4 deletions Alignment/CocoaModel/src/CocoaDaqReaderRoot.cc
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@
#include "CondFormats/OptAlignObjects/interface/OpticalAlignMeasurements.h"

#include <iostream>
#include <string>

#include "TClonesArray.h"

Expand Down Expand Up @@ -128,7 +129,7 @@ OpticalAlignMeasurementInfo CocoaDaqReaderRoot::GetMeasFromPosition2D( AliDaqPos
OpticalAlignMeasurementInfo meas;

meas.type_ = "SENSOR2D";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()));

meas.name_ = pos2D->GetID();
meas.name_ = std::string(pos2D->GetID().Data());
//- std::vector<std::string> measObjectNames_;
std::vector<bool> isSimu;
for( size_t jj = 0; jj < 2; jj++ ){
Expand Down Expand Up @@ -160,7 +161,7 @@ OpticalAlignMeasurementInfo CocoaDaqReaderRoot::GetMeasFromPositionCOPS( AliDaqP
OpticalAlignMeasurementInfo meas;

meas.type_ = "COPS";
meas.name_ = posCOPS->GetID();
meas.name_ = std::string(posCOPS->GetID().Data());
//- std::vector<std::string> measObjectNames_;
std::vector<bool> isSimu;
for( size_t jj = 0; jj < 4; jj++ ){
Expand Down Expand Up @@ -205,7 +206,7 @@ OpticalAlignMeasurementInfo CocoaDaqReaderRoot::GetMeasFromTilt( AliDaqTilt* til
OpticalAlignMeasurementInfo meas;

meas.type_ = "TILTMETER";
meas.name_ = tilt->GetID();
meas.name_ = std::string(tilt->GetID().Data());
//- std::vector<std::string> measObjectNames_;
std::vector<bool> isSimu;
for( size_t jj = 0; jj < 2; jj++ ){
Expand All @@ -232,7 +233,7 @@ OpticalAlignMeasurementInfo CocoaDaqReaderRoot::GetMeasFromDist( AliDaqDistance*
OpticalAlignMeasurementInfo meas;

meas.type_ = "DISTANCEMETER";
meas.name_ = dist->GetID();
meas.name_ = std::string(dist->GetID().Data());
//- std::vector<std::string> measObjectNames_;
std::vector<bool> isSimu;
for( size_t jj = 0; jj < 2; jj++ ){
Expand Down
2 changes: 1 addition & 1 deletion Alignment/CommonAlignmentMonitor/plugins/AlignmentMonitorGeneric.cc
View file
Open in desktop
Expand Up @@ -55,7 +55,7 @@ void AlignmentMonitorGeneric::book()
alignableObjectId.idToString(type),
id, DetId(id).subdetId());

hists[n] = book1D(std::string("/iterN/") + std::string(name) + std::string("/"), std::string(histName), std::string(histTitle), nBin_, -5., 5.);
hists[n] = book1D(std::string("/iterN/") + std::string(name) + std::string("/"), std::string(histName.Data()), std::string(histTitle.Data()), nBin_, -5., 5.);
}
}

Expand Down
2 changes: 1 addition & 1 deletion Alignment/HIPAlignmentAlgorithm/src/HIPAlignmentAlgorithm.cc
View file
Open in desktop
Expand Up @@ -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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did you get this number from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous, compiler error/notes tell the potential buffer size.

tname.Append("_");
tname.Append(iterString);
Expand Down
2 changes: 1 addition & 1 deletion Alignment/OfflineValidation/plugins/MuonAlignmentAnalyzer.cc
View file
Open in desktop
Expand Up @@ -837,7 +837,7 @@ void MuonAlignmentAnalyzer::endJob(){



char binLabel[15];
char binLabel[40];

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did you get this number from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

More here: https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc6_amd64_gcc700/CMSSW_9_0_X_2017-02-10-1100/Alignment/OfflineValidation

for(unsigned int i=0 ; i<unitsLocalX.size() ; i++)
{
Expand Down