diff --git a/include/sdf/Element.hh b/include/sdf/Element.hh index 4850cd289..810ce7a49 100644 --- a/include/sdf/Element.hh +++ b/include/sdf/Element.hh @@ -206,7 +206,7 @@ namespace sdf const std::string &_type, const std::string &_defaultvalue, bool _required, - const std::string &_description=""); + const std::string &_description = ""); /// \brief Add a value to this Element. /// \param[in] _type Type of data the attribute will hold. @@ -216,7 +216,7 @@ namespace sdf /// \throws sdf::AssertionInternalError if an invalid type is given. public: void AddValue(const std::string &_type, const std::string &_defaultValue, bool _required, - const std::string &_description=""); + const std::string &_description = ""); /// \brief Get the param of an attribute. /// \param[in] _key the name of the attribute. @@ -499,7 +499,7 @@ namespace sdf const std::string &_type, const std::string &_defaultValue, bool _required, - const std::string &_description=""); + const std::string &_description = ""); /// \brief Private data pointer diff --git a/src/Converter_TEST.cc b/src/Converter_TEST.cc index cdee13f2e..0bbbbf091 100644 --- a/src/Converter_TEST.cc +++ b/src/Converter_TEST.cc @@ -1885,10 +1885,16 @@ TEST(Converter, MuchNewerVersion) ASSERT_TRUE(sdf::Converter::Convert(&xmlDoc, "1.6")); } -const std::string CONVERT_DOC_15_16 = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "sdf", "1.6", "1_5.convert"); -const std::string CONVERT_DOC_16_17 = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "sdf", "1.7", "1_6.convert"); +static std::string ConvertDoc_15_16() +{ + return sdf::filesystem::append(PROJECT_SOURCE_PATH, "sdf", "1.6", + "1_5.convert"); +} +static std::string ConvertDoc_16_17() +{ + return sdf::filesystem::append(PROJECT_SOURCE_PATH, "sdf", "1.7", + "1_6.convert"); +} ///////////////////////////////////////////////// /// Test conversion of imu in 1.5 to 1.6 @@ -1930,7 +1936,7 @@ TEST(Converter, IMU_15_to_16) // Convert TiXmlDocument convertXmlDoc; - convertXmlDoc.LoadFile(CONVERT_DOC_15_16); + convertXmlDoc.LoadFile(ConvertDoc_15_16()); sdf::Converter::Convert(&xmlDoc, &convertXmlDoc); // Check some basic elements @@ -2024,7 +2030,7 @@ TEST(Converter, World_15_to_16) // Convert TiXmlDocument convertXmlDoc; - convertXmlDoc.LoadFile(CONVERT_DOC_15_16); + convertXmlDoc.LoadFile(ConvertDoc_15_16()); sdf::Converter::Convert(&xmlDoc, &convertXmlDoc); // Check some basic elements @@ -2080,7 +2086,7 @@ TEST(Converter, Pose_16_to_17) // Convert TiXmlDocument convertXmlDoc; - convertXmlDoc.LoadFile(CONVERT_DOC_16_17); + convertXmlDoc.LoadFile(ConvertDoc_16_17()); sdf::Converter::Convert(&xmlDoc, &convertXmlDoc); // Check some basic elements diff --git a/src/Error.cc b/src/Error.cc index 1793ff238..e7059b784 100644 --- a/src/Error.cc +++ b/src/Error.cc @@ -39,7 +39,6 @@ std::string Error::Message() const } ///////////////////////////////////////////////// -// cppcheck-suppress unusedFunction Error::operator bool() const { return this->code != ErrorCode::NONE; diff --git a/src/FrameSemantics.cc b/src/FrameSemantics.cc index 323303001..86ac7ae53 100644 --- a/src/FrameSemantics.cc +++ b/src/FrameSemantics.cc @@ -15,6 +15,7 @@ * */ #include +#include #include #include diff --git a/src/Joint.cc b/src/Joint.cc index 55c79d98b..78731090d 100644 --- a/src/Joint.cc +++ b/src/Joint.cc @@ -67,7 +67,6 @@ class sdf::JointPrivate public: double threadPitch = 1.0; /// \brief Joint axis - // cppcheck-suppress public: std::array, 2> axis; /// \brief The SDF element pointer used during load. diff --git a/src/Light.cc b/src/Light.cc index fed94919c..f2944d3b4 100644 --- a/src/Light.cc +++ b/src/Light.cc @@ -122,7 +122,7 @@ Light &Light::operator=(Light &&_light) ////////////////////////////////////////////////// void Light::CopyFrom(const Light &_light) { - this->dataPtr->name= _light.dataPtr->name; + this->dataPtr->name = _light.dataPtr->name; this->dataPtr->pose = _light.dataPtr->pose; this->dataPtr->poseRelativeTo = _light.dataPtr->poseRelativeTo; this->dataPtr->type = _light.dataPtr->type; @@ -251,7 +251,7 @@ Errors Light::Load(ElementPtr _sdf) this->SetSpotInnerAngle(doubleValue.first); // Check for and set outer_angle - doubleValue =spotElem->Get( + doubleValue = spotElem->Get( "outer_angle", this->dataPtr->spotOuterAngle.Radian()); if (!doubleValue.second) { @@ -261,7 +261,7 @@ Errors Light::Load(ElementPtr _sdf) this->SetSpotOuterAngle(doubleValue.first); // Check for and set falloff - doubleValue =spotElem->Get("falloff", this->dataPtr->spotFalloff); + doubleValue = spotElem->Get("falloff", this->dataPtr->spotFalloff); if (!doubleValue.second) { errors.push_back({ErrorCode::ELEMENT_MISSING, diff --git a/src/Link.cc b/src/Link.cc index 93532bdc8..0a4f7fdd5 100644 --- a/src/Link.cc +++ b/src/Link.cc @@ -490,5 +490,5 @@ bool Link::EnableWind() const ///////////////////////////////////////////////// void Link::SetEnableWind(const bool _enableWind) { - this->dataPtr->enableWind =_enableWind; + this->dataPtr->enableWind = _enableWind; } diff --git a/src/Model.cc b/src/Model.cc index e7e1ce759..c74d1752f 100644 --- a/src/Model.cc +++ b/src/Model.cc @@ -495,7 +495,7 @@ bool Model::EnableWind() const ///////////////////////////////////////////////// void Model::SetEnableWind(const bool _enableWind) { - this->dataPtr->enableWind =_enableWind; + this->dataPtr->enableWind = _enableWind; } ///////////////////////////////////////////////// diff --git a/src/Noise.cc b/src/Noise.cc index 7b0c77ca3..d00c1b948 100644 --- a/src/Noise.cc +++ b/src/Noise.cc @@ -123,7 +123,7 @@ Errors Noise::Load(ElementPtr _sdf) else if (typeLower == "gaussian") this->dataPtr->type = NoiseType::GAUSSIAN; else if (typeLower == "gaussian_quantized") - this->dataPtr->type =NoiseType::GAUSSIAN_QUANTIZED; + this->dataPtr->type = NoiseType::GAUSSIAN_QUANTIZED; else { errors.push_back({ErrorCode::ELEMENT_MISSING, diff --git a/src/Pbr.cc b/src/Pbr.cc index 63fd5800c..1643ab1be 100644 --- a/src/Pbr.cc +++ b/src/Pbr.cc @@ -206,7 +206,7 @@ Errors PbrWorkflow::Load(sdf::ElementPtr _sdf) this->dataPtr->albedoMap = _sdf->Get("albedo_map", this->dataPtr->albedoMap).first; - this->dataPtr->normalMap= _sdf->Get("normal_map", + this->dataPtr->normalMap = _sdf->Get("normal_map", this->dataPtr->normalMap).first; this->dataPtr->environmentMap = _sdf->Get("environment_map", diff --git a/src/SDF.cc b/src/SDF.cc index 48c1943b3..673bf15a2 100644 --- a/src/SDF.cc +++ b/src/SDF.cc @@ -46,7 +46,9 @@ static URIPathMap g_uriPathMap; static std::function g_findFileCB; -std::string SDF::version = SDF_VERSION; +// TODO(azeey) This violates the Google style guide. Change to a function that +// returns the version string when possible. +std::string SDF::version = SDF_VERSION; // NOLINT(runtime/string) ///////////////////////////////////////////////// // cppcheck-suppress passedByValue @@ -325,9 +327,7 @@ void SDF::PrintDoc() std::cout << "\n"; - std::cout << "\ - \ - \n"; + std::cout << " \n"; } ///////////////////////////////////////////////// diff --git a/src/Scene.cc b/src/Scene.cc index a042a1b07..de532b156 100644 --- a/src/Scene.cc +++ b/src/Scene.cc @@ -128,7 +128,7 @@ Errors Scene::Load(ElementPtr _sdf) } // Get the ambient property - this->dataPtr->ambient= _sdf->Get("ambient", + this->dataPtr->ambient = _sdf->Get("ambient", this->dataPtr->ambient).first; // Get the background color property diff --git a/src/Utils.hh b/src/Utils.hh index 73740ed1d..9c69bb699 100644 --- a/src/Utils.hh +++ b/src/Utils.hh @@ -19,6 +19,7 @@ #include #include +#include #include #include "sdf/Error.hh" #include "sdf/Element.hh" diff --git a/src/ign_TEST.cc b/src/ign_TEST.cc index 46a0b0143..fbdc34582 100644 --- a/src/ign_TEST.cc +++ b/src/ign_TEST.cc @@ -30,9 +30,15 @@ #define pclose _pclose #endif -static const std::string g_sdfVersion(" --force-version " + - std::string(SDF_VERSION_FULL)); -static const std::string g_ignCommand(std::string(IGN_PATH) + "/ign"); +static std::string SdfVersion() +{ + return " --force-version " + std::string(SDF_VERSION_FULL); +} + +static std::string IgnCommand() +{ + return std::string(IGN_PATH) + "/ign"; +} ///////////////////////////////////////////////// std::string custom_exec_str(std::string _cmd) @@ -68,7 +74,7 @@ TEST(check, SDF) // Check box_plane_low_friction_test.world std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output) << output; } @@ -78,7 +84,7 @@ TEST(check, SDF) // Check box_bad_test.world std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Required attribute"), std::string::npos) << output; } @@ -90,7 +96,7 @@ TEST(check, SDF) // Check world_duplicate.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: World with name[default] already exists."), std::string::npos) << output; } @@ -102,7 +108,7 @@ TEST(check, SDF) // Check world_sibling_same_names.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: Non-unique names"), std::string::npos) << output; } @@ -114,7 +120,7 @@ TEST(check, SDF) // Check model_duplicate_links.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: link with name[link] already exists."), std::string::npos) << output; } @@ -126,7 +132,7 @@ TEST(check, SDF) // Check model_duplicate_joints.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: joint with name[joint] already exists."), std::string::npos) << output; } @@ -138,7 +144,7 @@ TEST(check, SDF) // Check model_link_joint_same_name.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: Non-unique names"), std::string::npos) << output; } @@ -150,7 +156,7 @@ TEST(check, SDF) // Check link_duplicate_sibling_collisions.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: collision with name[collision] " "already exists."), std::string::npos) << output; @@ -163,7 +169,7 @@ TEST(check, SDF) // Check link_duplicate_sibling_visuals.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: visual with name[visual] already exists."), std::string::npos) << output; } @@ -175,7 +181,7 @@ TEST(check, SDF) // Check link_duplicate_cousin_collisions.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output) << output; } @@ -186,7 +192,7 @@ TEST(check, SDF) // Check link_duplicate_cousin_visuals.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output) << output; } @@ -196,7 +202,7 @@ TEST(check, SDF) // Check joint_invalid_child.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: Child link with name[invalid] specified by " "joint with name[joint] not found in model with " "name[joint_invalid_child]."), @@ -209,7 +215,7 @@ TEST(check, SDF) // Check joint_invalid_parent.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: parent link with name[invalid] specified by " "joint with name[joint] not found in model with " "name[joint_invalid_parent]."), @@ -222,7 +228,7 @@ TEST(check, SDF) // Check joint_invalid_parent_same_as_child.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: Joint with name[joint] must " "specify different link names for parent and child, " "while [link] was specified for both."), @@ -235,7 +241,7 @@ TEST(check, SDF) // Check joint_child_world.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: Joint with name[joint] specified invalid " "child link [world]."), std::string::npos) << output; @@ -248,7 +254,7 @@ TEST(check, SDF) // Check joint_parent_world.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output) << output; } @@ -259,7 +265,7 @@ TEST(check, SDF) // Check model_canonical_link.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output) << output; } @@ -269,7 +275,7 @@ TEST(check, SDF) // Check model_invalid_canonical_link.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: canonical_link with name[link3] not found in " "model with name[model_invalid_canonical_link]."), std::string::npos) << output; @@ -281,7 +287,7 @@ TEST(check, SDF) // Check model_without_links.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: A model must have at least one link."), std::string::npos) << output; } @@ -292,7 +298,7 @@ TEST(check, SDF) // Check nested_model.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output) << output; } @@ -302,7 +308,7 @@ TEST(check, SDF) // Check nested_canonical_link.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output) << output; } @@ -314,7 +320,7 @@ TEST(check, SDF) // Check nested_invalid_explicit_canonical_link.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: canonical_link with name[nested::link] not " "found in model with name[top]."), std::string::npos) << output; @@ -328,7 +334,7 @@ TEST(check, SDF) // Check model_invalid_reserved_names.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: The supplied link name [world] is reserved."), std::string::npos) << output; EXPECT_NE(output.find("Error: The supplied link name [__link__] " @@ -353,7 +359,7 @@ TEST(check, SDF) std::string path = pathBase +"/ignore_sdf_in_plugin.sdf"; std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output) << output; } @@ -362,7 +368,7 @@ TEST(check, SDF) std::string path = pathBase +"/ignore_sdf_in_namespaced_elements.sdf"; std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output) << output; } @@ -373,7 +379,7 @@ TEST(check, SDF) // Check model_frame_attached_to.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output) << output; } @@ -384,7 +390,7 @@ TEST(check, SDF) // Check model_frame_attached_to_joint.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output) << output; } @@ -395,7 +401,7 @@ TEST(check, SDF) // Check model_frame_attached_to_nested_model.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output) << output; } @@ -405,7 +411,7 @@ TEST(check, SDF) // Check model_frame_invalid_attached_to.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: attached_to name[A] specified by frame with " "name[F3] does not match a nested model, link, " "joint, or frame name in model with " @@ -423,7 +429,7 @@ TEST(check, SDF) // Check model_frame_invalid_attached_to_cycle.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(std::string::npos, output.find( "FrameAttachedToGraph cycle detected, already visited vertex [F1].")) << output; @@ -439,7 +445,7 @@ TEST(check, SDF) // Check world_frame_attached_to.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output) << output; } @@ -449,7 +455,7 @@ TEST(check, SDF) // Check world_frame_invalid_attached_to.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: attached_to name[A] specified by frame with " "name[F] does not match a model or frame " "name in world with " @@ -469,7 +475,7 @@ TEST(check, SDF) // Check model_link_relative_to.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output) << output; } @@ -479,7 +485,7 @@ TEST(check, SDF) // Check model_invalid_link_relative_to.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: relative_to name[A] specified by link with " "name[L] does not match a nested model, link, " "joint, or frame name in model with " @@ -498,7 +504,7 @@ TEST(check, SDF) // Check model_nested_model_relative_to.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output) << output; } @@ -509,7 +515,7 @@ TEST(check, SDF) // Check model_invalid_nested_joint_child.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: Child link with name[M::C] specified by " "joint with name[J] not found in model with " "name[model_invalid_nested_joint_child]."), @@ -523,7 +529,7 @@ TEST(check, SDF) // Check model_joint_relative_to.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output) << output; } @@ -533,7 +539,7 @@ TEST(check, SDF) // Check model_invalid_joint_relative_to.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: relative_to name[A] specified by joint with " "name[J] does not match a nested model, link, " "joint, or frame name in model with " @@ -552,7 +558,7 @@ TEST(check, SDF) // Check model_frame_relative_to.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output) << output; } @@ -563,7 +569,7 @@ TEST(check, SDF) // Check model_frame_relative_to_joint.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output) << output; } @@ -573,7 +579,7 @@ TEST(check, SDF) // Check model_invalid_frame_relative_to.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: relative_to name[A] specified by frame with " "name[F] does not match a nested model, link, " "joint, or frame name in model with " @@ -591,7 +597,7 @@ TEST(check, SDF) // Check model_invalid_frame_relative_to_cycle.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(std::string::npos, output.find( "PoseRelativeToGraph cycle detected, already visited vertex [F1].")) << output; @@ -607,7 +613,7 @@ TEST(check, SDF) // Check world_frame_relative_to.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output) << output; } @@ -617,7 +623,7 @@ TEST(check, SDF) // Check world_frame_invalid_relative_to.sdf std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_NE(output.find("Error: relative_to name[A] specified by model with " "name[M] does not match a model or frame " "name in world with " @@ -652,7 +658,7 @@ TEST(check_model_sdf, SDF) std::string path = pathBase +"/model.sdf"; std::string output = - custom_exec_str(g_ignCommand + " sdf -k " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output); } @@ -662,7 +668,7 @@ TEST(check_model_sdf, SDF) std::string output = custom_exec_str("cd " + pathBase + " && " + - g_ignCommand + " sdf -k " + path + g_sdfVersion); + IgnCommand() + " sdf -k " + path + SdfVersion()); EXPECT_EQ("Valid.\n", output); } } @@ -672,7 +678,7 @@ TEST(describe, SDF) { // Get the description std::string output = - custom_exec_str(g_ignCommand + " sdf -d " + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -d " + SdfVersion()); EXPECT_FALSE(output.empty()); // The first line should start with the following text. @@ -694,7 +700,7 @@ TEST(print, SDF) // Check box_plane_low_friction_test.world std::string output = - custom_exec_str(g_ignCommand + " sdf -p " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -p " + path + SdfVersion()); EXPECT_EQ(sdf->Root()->ToString(""), output); } @@ -704,7 +710,7 @@ TEST(print, SDF) // Check box_bad_test.world std::string output = - custom_exec_str(g_ignCommand + " sdf -p " + path + g_sdfVersion); + custom_exec_str(IgnCommand() + " sdf -p " + path + SdfVersion()); EXPECT_TRUE(output.find("Required attribute") != std::string::npos); } } diff --git a/src/parser.cc b/src/parser.cc index 06a3b89f5..7208af789 100644 --- a/src/parser.cc +++ b/src/parser.cc @@ -1061,10 +1061,10 @@ bool readXml(TiXmlElement *_xml, ElementPtr _sdf, Errors &_errors) { includeSDF->Root()->GetFirstElement()->SetParent(_sdf); _sdf->InsertElement(includeSDF->Root()->GetFirstElement()); - // TODO: This was used to store the included filename so that when - // a world is saved, the included model's SDF is not stored in the - // world file. This highlights the need to make model inclusion - // a core feature of SDF, and not a hack that that parser handles + // TODO(anyone): This was used to store the included filename so that + // when a world is saved, the included model's SDF is not stored in + // the world file. This highlights the need to make model inclusion a + // core feature of SDF, and not a hack that that parser handles // includeSDF->Root()->GetFirstElement()->SetInclude( // elemXml->Attribute("filename")); } diff --git a/src/parser_urdf.cc b/src/parser_urdf.cc index 1dd1f2d62..2480ad0b1 100644 --- a/src/parser_urdf.cc +++ b/src/parser_urdf.cc @@ -49,9 +49,9 @@ typedef std::map > StringSDFExtensionPtrMap g_extensions; bool g_reduceFixedJoints; bool g_enforceLimits; -std::string g_collisionExt = "_collision"; -std::string g_visualExt = "_visual"; -std::string g_lumpPrefix = "_fixed_joint_lump__"; +const char kCollisionExt[] = "_collision"; +const char kVisualExt[] = "_visual"; +const char kLumpPrefix[] = "_fixed_joint_lump__"; urdf::Pose g_initialRobotPose; bool g_initialRobotPoseValid = false; std::set g_fixedJointsTransformedInRevoluteJoints; @@ -916,8 +916,8 @@ void ReduceVisualsToParent(urdf::LinkSharedPtr _link) // from another descendant link connected by a fixed joint. // // Algorithm for generating new name (or group name) is: - // original name + g_lumpPrefix+original link name (urdf 0.3.x) - // original group name + g_lumpPrefix+original link name (urdf 0.2.x) + // original name + kLumpPrefix+original link name (urdf 0.3.x) + // original group name + kLumpPrefix+original link name (urdf 0.2.x) // The purpose is to track where this visual came from // (original parent link name before lumping/reducing). for (std::vector::iterator @@ -926,7 +926,7 @@ void ReduceVisualsToParent(urdf::LinkSharedPtr _link) { // 20151116: changelog for pull request #235 std::string newVisualName; - std::size_t lumpIndex = (*visualIt)->name.find(g_lumpPrefix); + std::size_t lumpIndex = (*visualIt)->name.find(kLumpPrefix); if (lumpIndex != std::string::npos) { newVisualName = (*visualIt)->name; @@ -976,8 +976,8 @@ void ReduceCollisionsToParent(urdf::LinkSharedPtr _link) // from another descendant link connected by a fixed joint. // // Algorithm for generating new name (or group name) is: - // original name + g_lumpPrefix+original link name (urdf 0.3.x) - // original group name + g_lumpPrefix+original link name (urdf 0.2.x) + // original name + kLumpPrefix+original link name (urdf 0.3.x) + // original group name + kLumpPrefix+original link name (urdf 0.2.x) // The purpose is to track where this collision came from // (original parent link name before lumping/reducing). for (std::vector::iterator @@ -985,7 +985,7 @@ void ReduceCollisionsToParent(urdf::LinkSharedPtr _link) collisionIt != _link->collision_array.end(); ++collisionIt) { std::string newCollisionName; - std::size_t lumpIndex = (*collisionIt)->name.find(g_lumpPrefix); + std::size_t lumpIndex = (*collisionIt)->name.find(kLumpPrefix); if (lumpIndex != std::string::npos) { newCollisionName = (*collisionIt)->name; @@ -1075,9 +1075,6 @@ URDF2SDF::URDF2SDF() g_enforceLimits = true; g_reduceFixedJoints = true; g_extensions.clear(); - g_collisionExt = "_collision"; - g_visualExt = "_visual"; - g_lumpPrefix = "_fixed_joint_lump__"; g_initialRobotPoseValid = false; g_fixedJointsTransformedInRevoluteJoints.clear(); g_fixedJointsTransformedInFixedJoints.clear(); @@ -1592,8 +1589,8 @@ void InsertSDFExtensionCollision(TiXmlElement *_elem, // << "]\n"; // std::cerr << "----------------------------\n"; - std::string lumpCollisionName = g_lumpPrefix + - (*ge)->oldLinkName + g_collisionExt; + std::string lumpCollisionName = kLumpPrefix + + (*ge)->oldLinkName + kCollisionExt; bool wasReduced = (_linkName == (*ge)->oldLinkName); bool collisionNameContainsLinkname = @@ -1601,7 +1598,7 @@ void InsertSDFExtensionCollision(TiXmlElement *_elem, bool collisionNameContainsLumpedLinkname = sdfCollisionName.find(lumpCollisionName) != std::string::npos; bool collisionNameContainsLumpedRef = - sdfCollisionName.find(g_lumpPrefix) != std::string::npos; + sdfCollisionName.find(kLumpPrefix) != std::string::npos; if (!collisionNameContainsLinkname) { @@ -1610,9 +1607,9 @@ void InsertSDFExtensionCollision(TiXmlElement *_elem, } // if the collision _elem was not reduced, - // its name should not have g_lumpPrefix in it. + // its name should not have kLumpPrefix in it. // otherwise, its name should have - // "g_lumpPrefix+[original link name before reduction]". + // "kLumpPrefix+[original link name before reduction]". if ((wasReduced && !collisionNameContainsLumpedRef) || (!wasReduced && collisionNameContainsLumpedLinkname)) { @@ -1906,8 +1903,8 @@ void InsertSDFExtensionVisual(TiXmlElement *_elem, // << "]\n"; // std::cerr << "----------------------------\n"; - std::string lumpVisualName = g_lumpPrefix + - (*ge)->oldLinkName + g_visualExt; + std::string lumpVisualName = kLumpPrefix + + (*ge)->oldLinkName + kVisualExt; bool wasReduced = (_linkName == (*ge)->oldLinkName); bool visualNameContainsLinkname = @@ -1915,7 +1912,7 @@ void InsertSDFExtensionVisual(TiXmlElement *_elem, bool visualNameContainsLumpedLinkname = sdfVisualName.find(lumpVisualName) != std::string::npos; bool visualNameContainsLumpedRef = - sdfVisualName.find(g_lumpPrefix) != std::string::npos; + sdfVisualName.find(kLumpPrefix) != std::string::npos; if (!visualNameContainsLinkname) { @@ -1924,9 +1921,9 @@ void InsertSDFExtensionVisual(TiXmlElement *_elem, } // if the visual _elem was not reduced, - // its name should not have g_lumpPrefix in it. + // its name should not have kLumpPrefix in it. // otherwise, its name should have - // "g_lumpPrefix+[original link name before reduction]". + // "kLumpPrefix+[original link name before reduction]". if ((wasReduced && !visualNameContainsLumpedRef) || (!wasReduced && visualNameContainsLumpedLinkname)) { @@ -2804,7 +2801,7 @@ void CreateCollisions(TiXmlElement* _elem, } // add _collision extension - collisionName = collisionName + g_collisionExt; + collisionName = collisionName + kCollisionExt; if (collisionCount > 0) { @@ -2849,7 +2846,7 @@ void CreateVisuals(TiXmlElement* _elem, } // add _visual extension - visualName = visualName + g_visualExt; + visualName = visualName + kVisualExt; if (visualCount > 0) { @@ -3098,7 +3095,7 @@ void CreateCollision(TiXmlElement* _elem, urdf::LinkConstSharedPtr _link, else { sdfCollision->SetAttribute("name", _link->name - + g_lumpPrefix + _oldLinkName); + + kLumpPrefix + _oldLinkName); } // std::cerr << "collision [" << sdfCollision->Attribute("name") << "]\n"; @@ -3144,7 +3141,7 @@ void CreateVisual(TiXmlElement *_elem, urdf::LinkConstSharedPtr _link, } else { - sdfVisual->SetAttribute("name", _link->name + g_lumpPrefix + _oldLinkName); + sdfVisual->SetAttribute("name", _link->name + kLumpPrefix + _oldLinkName); } // add the visualisation transfrom @@ -3446,12 +3443,12 @@ void ReduceSDFExtensionContactSensorFrameReplace( if (collision) { if (GetKeyValueAsString(collision->ToElement()) == - linkName + g_collisionExt) + linkName + kCollisionExt) { contact->RemoveChild(collision); TiXmlElement* collisionNameKey = new TiXmlElement("collision"); std::ostringstream collisionNameStream; - collisionNameStream << parentLinkName << g_collisionExt + collisionNameStream << parentLinkName << kCollisionExt << "_" << linkName; TiXmlText* collisionNameTxt = new TiXmlText( collisionNameStream.str()); diff --git a/src/parser_urdf_TEST.cc b/src/parser_urdf_TEST.cc index 8da81e31c..06e27c111 100644 --- a/src/parser_urdf_TEST.cc +++ b/src/parser_urdf_TEST.cc @@ -60,7 +60,7 @@ TEST(URDFParser, InitModelDoc_EmptyDoc_NoThrow) TiXmlDocument doc = TiXmlDocument(); sdf::URDF2SDF parser_; TiXmlDocument sdf_result = parser_.InitModelDoc(&doc); - ); // NOLINT(whitespace/parens) + ); SDF_SUPPRESS_DEPRECATED_END } @@ -74,7 +74,7 @@ TEST(URDFParser, InitModelDoc_BasicModel_NoThrow) doc.Parse(getMinimalUrdfTxt().c_str()); sdf::URDF2SDF parser_; TiXmlDocument sdf_result = parser_.InitModelDoc(&doc); - ); // NOLINT(whitespace/parens) + ); SDF_SUPPRESS_DEPRECATED_END } diff --git a/test/integration/audio.cc b/test/integration/audio.cc index 20c02e2b9..b8e7e09b4 100644 --- a/test/integration/audio.cc +++ b/test/integration/audio.cc @@ -23,13 +23,12 @@ #include "test_config.h" -const std::string SDF_TEST_FILE = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", - "audio.sdf"); TEST(SDFParser, AudioSDF_FullParameters_noThrow) { + const std::string sdfTestFile = sdf::filesystem::append( + PROJECT_SOURCE_PATH, "test", "integration", "audio.sdf"); sdf::SDFPtr p(new sdf::SDF()); sdf::init(p); - ASSERT_TRUE(sdf::readFile(SDF_TEST_FILE, p)); + ASSERT_TRUE(sdf::readFile(sdfTestFile , p)); } diff --git a/test/integration/cfm_damping_implicit_spring_damper.cc b/test/integration/cfm_damping_implicit_spring_damper.cc index 7ad41c38e..2c5cc090e 100644 --- a/test/integration/cfm_damping_implicit_spring_damper.cc +++ b/test/integration/cfm_damping_implicit_spring_damper.cc @@ -23,20 +23,16 @@ #include "test_config.h" -const std::string URDF_TEST_FILE = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", - "cfm_damping_implicit_spring_damper.urdf"); - -const std::string SDF_TEST_FILE = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", - "cfm_damping_implicit_spring_damper.sdf"); ///////////////////////////////////////////////// TEST(SDFParser, CFMDampingSDFTest) { + const std::string sdfTestFile = + sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", + "cfm_damping_implicit_spring_damper.sdf"); sdf::SDFPtr robot(new sdf::SDF()); sdf::init(robot); - ASSERT_TRUE(sdf::readFile(SDF_TEST_FILE, robot)); + ASSERT_TRUE(sdf::readFile(sdfTestFile, robot)); sdf::ElementPtr model = robot->Root()->GetElement("model"); unsigned int jointBitMask = 0; @@ -111,9 +107,13 @@ TEST(SDFParser, CFMDampingSDFTest) ///////////////////////////////////////////////// TEST(SDFParser, CFMDampingURDFTest) { + const std::string urdfTestFile = + sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", + "cfm_damping_implicit_spring_damper.urdf"); + sdf::SDFPtr robot(new sdf::SDF()); sdf::init(robot); - ASSERT_TRUE(sdf::readFile(URDF_TEST_FILE, robot)); + ASSERT_TRUE(sdf::readFile(urdfTestFile, robot)); sdf::ElementPtr root = robot->Root(); ASSERT_TRUE(root != nullptr); diff --git a/test/integration/converter.cc b/test/integration/converter.cc index a928376ef..00629be72 100644 --- a/test/integration/converter.cc +++ b/test/integration/converter.cc @@ -27,11 +27,6 @@ /// \brief Use different sdf versions for ParserStringConverter Test. void ParserStringConverter(const std::string &_version); -const std::string CONVERT_DOC_15_16 = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "sdf", "1.6", "1_5.convert"); -const std::string CONVERT_DOC_16_17 = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "sdf", "1.7", "1_6.convert"); - ///////////////////////////////////////////////// /// Test conversion using the parser sdf file converter interface. TEST(ConverterIntegration, ParserFileConverter) diff --git a/test/integration/disable_fixed_joint_reduction.cc b/test/integration/disable_fixed_joint_reduction.cc index 1c44aeb0f..383c3d24c 100644 --- a/test/integration/disable_fixed_joint_reduction.cc +++ b/test/integration/disable_fixed_joint_reduction.cc @@ -23,14 +23,6 @@ #include "test_config.h" -const std::string SDF_FIXED_JNT = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", - "fixed_joint_reduction.urdf"); - -const std::string SDF_FIXED_JNT_NO_LUMPING = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", - "fixed_joint_reduction_disabled.urdf"); - ///////////////////////////////////////////////// bool findJointInModel(const std::string &desired_joint_name, sdf::SDFPtr robot) { @@ -53,13 +45,20 @@ bool findJointInModel(const std::string &desired_joint_name, sdf::SDFPtr robot) ///////////////////////////////////////////////// TEST(SDFParser, DisableFixedJointReductionTest) { + const std::string sdfFixedJntFile = sdf::filesystem::append( + PROJECT_SOURCE_PATH, "test", "integration", "fixed_joint_reduction.urdf"); + + const std::string sdfFixedJntNoLumpingFile = + sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", + "fixed_joint_reduction_disabled.urdf"); + sdf::SDFPtr robot(new sdf::SDF()); sdf::init(robot); - ASSERT_TRUE(sdf::readFile(SDF_FIXED_JNT, robot)); + ASSERT_TRUE(sdf::readFile(sdfFixedJntFile, robot)); sdf::SDFPtr robot_disable_lumping(new sdf::SDF()); sdf::init(robot_disable_lumping); - ASSERT_TRUE(sdf::readFile(SDF_FIXED_JNT_NO_LUMPING, robot_disable_lumping)); + ASSERT_TRUE(sdf::readFile(sdfFixedJntNoLumpingFile, robot_disable_lumping)); ASSERT_FALSE(findJointInModel("joint12a", robot)); ASSERT_FALSE(findJointInModel("joint23a", robot)); diff --git a/test/integration/element_memory_leak.cc b/test/integration/element_memory_leak.cc index aee5f7b8e..00caa6408 100644 --- a/test/integration/element_memory_leak.cc +++ b/test/integration/element_memory_leak.cc @@ -69,12 +69,12 @@ const std::string sdfString( " \n" ""); -const std::string getMemInfoPath = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "tools", "get_mem_info.py"); -const std::string pythonMeminfo("python3 " + getMemInfoPath); - int getMemoryUsage() { + static const std::string getMemInfoPath = + sdf::filesystem::append(PROJECT_SOURCE_PATH, "tools", "get_mem_info.py"); + static const std::string pythonMeminfo("python3 " + getMemInfoPath); + return std::stoi(custom_exec(pythonMeminfo)); } diff --git a/test/integration/fixed_joint_reduction.cc b/test/integration/fixed_joint_reduction.cc index 5fe3cd7a8..0b1f828d1 100644 --- a/test/integration/fixed_joint_reduction.cc +++ b/test/integration/fixed_joint_reduction.cc @@ -24,30 +24,28 @@ #include "test_config.h" -const std::string SDF_TEST_FILE = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", - "fixed_joint_reduction.urdf"); -const std::string SDF_TEST_FILE_COLLISION = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", - "fixed_joint_reduction_collision.urdf"); -const std::string SDF_TEST_FILE_SIMPLE = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", - "fixed_joint_reduction_simple.urdf"); -const std::string SDF_TEST_FILE_VISUAL = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", - "fixed_joint_reduction_visual.urdf"); -const std::string SDF_TEST_FILE_COLLISION_VISUAL_EXTENSION = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", - "fixed_joint_reduction_collision_visual_extension.urdf"); -const std::string SDF_TEST_FILE_COLLISION_VISUAL_EXTENSION_SDF = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", - "fixed_joint_reduction_collision_visual_extension.sdf"); -const std::string SDF_TEST_FILE_COLLISION_VISUAL_EXTENSION_EMPTY_ROOT = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", - "fixed_joint_reduction_collision_visual_empty_root.urdf"); -const std::string SDF_TEST_FILE_COLLISION_VISUAL_EXTENSION_EMPTY_ROOT_SDF = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", - "fixed_joint_reduction_collision_visual_empty_root.sdf"); +const char SDF_TEST_FILE[] = + "fixed_joint_reduction.urdf"; +const char SDF_TEST_FILE_COLLISION[] = + "fixed_joint_reduction_collision.urdf"; +const char SDF_TEST_FILE_SIMPLE[] = + "fixed_joint_reduction_simple.urdf"; +const char SDF_TEST_FILE_VISUAL[] = + "fixed_joint_reduction_visual.urdf"; +const char SDF_TEST_FILE_COLLISION_VISUAL_EXTENSION[] = + "fixed_joint_reduction_collision_visual_extension.urdf"; +const char SDF_TEST_FILE_COLLISION_VISUAL_EXTENSION_SDF[] = + "fixed_joint_reduction_collision_visual_extension.sdf"; +const char SDF_TEST_FILE_COLLISION_VISUAL_EXTENSION_EMPTY_ROOT[] = + "fixed_joint_reduction_collision_visual_empty_root.urdf"; +const char SDF_TEST_FILE_COLLISION_VISUAL_EXTENSION_EMPTY_ROOT_SDF[] = + "fixed_joint_reduction_collision_visual_empty_root.sdf"; + +static std::string GetFullTestFilePath(const char *_input) +{ + return sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", + _input); +} const double gc_tolerance = 1e-6; @@ -60,7 +58,7 @@ void FixedJointReductionCollisionVisualExtensionEmptyRoot( ///////////////////////////////////////////////// TEST(SDFParser, FixedJointReductionEquivalenceTest) { - FixedJointReductionEquivalence(SDF_TEST_FILE); + FixedJointReductionEquivalence(GetFullTestFilePath(SDF_TEST_FILE)); } ///////////////////////////////////////////////// @@ -69,7 +67,7 @@ TEST(SDFParser, FixedJointReductionEquivalenceTest) // version of urdfdom (see #63). TEST(SDFParser, FixedJointReductionVisualTest) { - FixedJointReductionEquivalence(SDF_TEST_FILE_VISUAL); + FixedJointReductionEquivalence(GetFullTestFilePath(SDF_TEST_FILE_VISUAL)); } ///////////////////////////////////////////////// @@ -78,7 +76,7 @@ TEST(SDFParser, FixedJointReductionVisualTest) // FixedJointReductionVisualTest TEST(SDFParser, FixedJointReductionCollisionTest) { - FixedJointReductionEquivalence(SDF_TEST_FILE_COLLISION); + FixedJointReductionEquivalence(GetFullTestFilePath(SDF_TEST_FILE_COLLISION)); } // this updated test will not work for old parser @@ -91,8 +89,8 @@ TEST(SDFParser, FixedJointReductionCollisionTest) TEST(SDFParser, FixedJointReductionCollisionVisualExtensionTest) { FixedJointReductionCollisionVisualExtension( - SDF_TEST_FILE_COLLISION_VISUAL_EXTENSION, - SDF_TEST_FILE_COLLISION_VISUAL_EXTENSION_SDF); + GetFullTestFilePath(SDF_TEST_FILE_COLLISION_VISUAL_EXTENSION), + GetFullTestFilePath(SDF_TEST_FILE_COLLISION_VISUAL_EXTENSION_SDF)); } ///////////////////////////////////////////////// @@ -103,8 +101,9 @@ TEST(SDFParser, FixedJointReductionCollisionVisualExtensionTest) TEST(SDFParser, FixedJointReductionCollisionVisualExtensionEmptyRootTest) { FixedJointReductionCollisionVisualExtensionEmptyRoot( - SDF_TEST_FILE_COLLISION_VISUAL_EXTENSION_EMPTY_ROOT, - SDF_TEST_FILE_COLLISION_VISUAL_EXTENSION_EMPTY_ROOT_SDF); + GetFullTestFilePath(SDF_TEST_FILE_COLLISION_VISUAL_EXTENSION_EMPTY_ROOT), + GetFullTestFilePath( + SDF_TEST_FILE_COLLISION_VISUAL_EXTENSION_EMPTY_ROOT_SDF)); } ///////////////////////////////////////////////// @@ -621,17 +620,17 @@ void FixedJointReductionEquivalence(const std::string &_file) } } - if (_file.compare(SDF_TEST_FILE) == 0) + if (_file.compare(GetFullTestFilePath(SDF_TEST_FILE)) == 0) { EXPECT_EQ(countCollisions, 7u); EXPECT_EQ(countVisuals, 7u); } - else if (_file.compare(SDF_TEST_FILE_COLLISION) == 0) + else if (_file.compare(GetFullTestFilePath(SDF_TEST_FILE_COLLISION)) == 0) { EXPECT_EQ(countCollisions, 6u); EXPECT_EQ(countVisuals, 0u); } - else if (_file.compare(SDF_TEST_FILE_VISUAL) == 0) + else if (_file.compare(GetFullTestFilePath(SDF_TEST_FILE_VISUAL)) == 0) { EXPECT_EQ(countCollisions, 0u); EXPECT_EQ(countVisuals, 6u); @@ -643,7 +642,7 @@ TEST(SDFParser, FixedJointReductionSimple) { sdf::SDFPtr robot(new sdf::SDF()); sdf::init(robot); - ASSERT_TRUE(sdf::readFile(SDF_TEST_FILE_SIMPLE, robot)); + ASSERT_TRUE(sdf::readFile(GetFullTestFilePath(SDF_TEST_FILE_SIMPLE), robot)); std::map linkMasses; std::map linkPoses; diff --git a/test/integration/force_torque_sensor.cc b/test/integration/force_torque_sensor.cc index d64aca6ac..12dc60ce1 100644 --- a/test/integration/force_torque_sensor.cc +++ b/test/integration/force_torque_sensor.cc @@ -23,16 +23,15 @@ #include "test_config.h" -const std::string SDF_TEST_FILE = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", - "force_torque_sensor.urdf"); - ///////////////////////////////////////////////// TEST(SDFParser, ForceTorqueSensorTest) { + const std::string sdfTestFile = sdf::filesystem::append( + PROJECT_SOURCE_PATH, "test", "integration", "force_torque_sensor.urdf"); + sdf::SDFPtr robot(new sdf::SDF()); sdf::init(robot); - ASSERT_TRUE(sdf::readFile(SDF_TEST_FILE, robot)); + ASSERT_TRUE(sdf::readFile(sdfTestFile, robot)); sdf::ElementPtr model = robot->Root()->GetElement("model"); unsigned int jointBitMask = 0; diff --git a/test/integration/locale_fix.cc b/test/integration/locale_fix.cc index a0ff21dfb..a6bf75184 100644 --- a/test/integration/locale_fix.cc +++ b/test/integration/locale_fix.cc @@ -25,15 +25,14 @@ #include "test_config.h" -const std::string SDF_TEST_FILE = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", - "numeric.sdf"); - // Windows supports the setlocale call but we can not extract the // available locales using the Linux call #ifndef _MSC_VER TEST(CheckFixForLocal, MakeTestToFail) { + const std::string sdfTestFile = sdf::filesystem::append( + PROJECT_SOURCE_PATH, "test", "integration", "numeric.sdf"); + // Check if any of the latin locales is avilable FILE *fp = popen("locale -a | grep '^es\\|^pt_\\|^it_' | head -n 1", "r"); @@ -60,7 +59,7 @@ TEST(CheckFixForLocal, MakeTestToFail) // fix to allow make test without make install sdf::SDFPtr p(new sdf::SDF()); sdf::init(p); - ASSERT_TRUE(sdf::readFile(SDF_TEST_FILE, p)); + ASSERT_TRUE(sdf::readFile(sdfTestFile, p)); sdf::ElementPtr elem = p->Root()->GetElement("world") ->GetElement("physics")->GetElement("ode")->GetElement("solver") @@ -70,8 +69,8 @@ TEST(CheckFixForLocal, MakeTestToFail) elem->Set(0.423); - // TODO: automatic checking. Error is thrown to the log file and std::err - // We should check for "Error [Param.cc:186] Unable to set value" + // TODO(anyone): automatic checking. Error is thrown to the log file and + // std::err We should check for "Error [Param.cc:186] Unable to set value" // Problem is How to get the log file path without duplicating code // Verify that the locale is not affecting the Param constructor diff --git a/test/integration/provide_feedback.cc b/test/integration/provide_feedback.cc index a9003a32f..e8ed95991 100644 --- a/test/integration/provide_feedback.cc +++ b/test/integration/provide_feedback.cc @@ -23,16 +23,15 @@ #include "test_config.h" -const std::string SDF_TEST_FILE = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", - "provide_feedback.urdf"); - ///////////////////////////////////////////////// TEST(SDFParser, ProvideFeedbackTest) { + const std::string sdfTestFile = sdf::filesystem::append( + PROJECT_SOURCE_PATH, "test", "integration", "provide_feedback.urdf"); + sdf::SDFPtr robot(new sdf::SDF()); sdf::init(robot); - ASSERT_TRUE(sdf::readFile(SDF_TEST_FILE, robot)); + ASSERT_TRUE(sdf::readFile(sdfTestFile, robot)); sdf::ElementPtr root = robot->Root(); ASSERT_TRUE(root != nullptr); diff --git a/test/integration/schema_test.cc b/test/integration/schema_test.cc index 028548471..4eab3dc2c 100644 --- a/test/integration/schema_test.cc +++ b/test/integration/schema_test.cc @@ -25,30 +25,17 @@ #include "test_config.h" -const std::string SDF_ROOT_SCHEMA = - sdf::filesystem::append(PROJECT_BINARY_DIR, "sdf", SDF_PROTOCOL_VERSION, - "root.xsd"); - -const std::string SDF_TEST_PR2 = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", "model", - "pr2.sdf"); - -const std::string SDF_TEST_TURTLEBOT = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", "model", - "turtlebot.sdf"); - -const std::string SDF_TEST_PENDULUM = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", "model", - "double_pendulum.sdf"); - class SDFSchemaGenerator : public testing::Test { public: void runXMLlint(const std::string & model) { + const std::string sdfRootSchema = sdf::filesystem::append( + PROJECT_BINARY_DIR, "sdf", SDF_PROTOCOL_VERSION, "root.xsd"); + std::string xmllintCmd = "xmllint --noout --schema " + - SDF_ROOT_SCHEMA + " " + model; + sdfRootSchema + " " + model; std::cout << "CMD[" << xmllintCmd << "]\n"; if (system(xmllintCmd.c_str()) != 0) { @@ -64,18 +51,28 @@ class SDFSchemaGenerator : public testing::Test ///////////////////////////////////////////////// TEST_F(SDFSchemaGenerator, TestDoublePendulum) { - runXMLlint(SDF_TEST_PENDULUM); + const std::string sdfTestPendulum = + sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", + "model", "double_pendulum.sdf"); + + runXMLlint(sdfTestPendulum); } ///////////////////////////////////////////////// TEST_F(SDFSchemaGenerator, TestPR2Model) { - runXMLlint(SDF_TEST_PR2); + const std::string sdfTestPr2 = sdf::filesystem::append( + PROJECT_SOURCE_PATH, "test", "integration", "model", "pr2.sdf"); + + runXMLlint(sdfTestPr2); } ///////////////////////////////////////////////// TEST_F(SDFSchemaGenerator, TestTurtleBotModel) { - runXMLlint(SDF_TEST_TURTLEBOT); + const std::string sdfTestTurtlebot = sdf::filesystem::append( + PROJECT_SOURCE_PATH, "test", "integration", "model", "turtlebot.sdf"); + + runXMLlint(sdfTestTurtlebot); } diff --git a/test/integration/sdf_custom.cc b/test/integration/sdf_custom.cc index f0bf349ff..63402c5ff 100644 --- a/test/integration/sdf_custom.cc +++ b/test/integration/sdf_custom.cc @@ -23,15 +23,14 @@ #include "test_config.h" -const std::string SDF_TEST_FILE = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", - "custom_elems_attrs.sdf"); - ///////////////////////////////////////////////// TEST(SDFParser, CustomElements) { + const std::string sdfTestFile = sdf::filesystem::append( + PROJECT_SOURCE_PATH, "test", "integration", "custom_elems_attrs.sdf"); + sdf::Root root; - EXPECT_TRUE(root.Load(SDF_TEST_FILE).empty()); + EXPECT_TRUE(root.Load(sdfTestFile).empty()); const sdf::World *world = root.WorldByIndex(0); diff --git a/test/integration/urdf_gazebo_extensions.cc b/test/integration/urdf_gazebo_extensions.cc index 6f2298dfb..80b064c8f 100644 --- a/test/integration/urdf_gazebo_extensions.cc +++ b/test/integration/urdf_gazebo_extensions.cc @@ -23,16 +23,16 @@ #include "test_config.h" -const std::string URDF_TEST_FILE = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", - "urdf_gazebo_extensions.urdf"); - ///////////////////////////////////////////////// TEST(SDFParser, UrdfGazeboExtensionURDFTest) { + const std::string urdfTestFile = + sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", + "urdf_gazebo_extensions.urdf"); + sdf::SDFPtr robot(new sdf::SDF()); sdf::init(robot); - ASSERT_TRUE(sdf::readFile(URDF_TEST_FILE, robot)); + ASSERT_TRUE(sdf::readFile(urdfTestFile, robot)); sdf::ElementPtr model = robot->Root()->GetElement("model"); for (sdf::ElementPtr joint = model->GetElement("joint"); joint; diff --git a/test/integration/urdf_joint_parameters.cc b/test/integration/urdf_joint_parameters.cc index 49d6caaa1..7b178cc54 100644 --- a/test/integration/urdf_joint_parameters.cc +++ b/test/integration/urdf_joint_parameters.cc @@ -23,16 +23,15 @@ #include "test_config.h" -const std::string SDF_TEST_FILE = - sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "integration", - "urdf_joint_parameters.urdf"); - ///////////////////////////////////////////////// TEST(SDFParser, JointAxisParameters) { + const std::string sdfTestFile = sdf::filesystem::append( + PROJECT_SOURCE_PATH, "test", "integration", "urdf_joint_parameters.urdf"); + sdf::SDFPtr robot(new sdf::SDF()); sdf::init(robot); - ASSERT_TRUE(sdf::readFile(SDF_TEST_FILE, robot)); + ASSERT_TRUE(sdf::readFile(sdfTestFile, robot)); sdf::ElementPtr model = robot->Root()->GetElement("model"); diff --git a/tools/cpplint.py b/tools/cpplint.py index 1847a26a3..1632bed19 100644 --- a/tools/cpplint.py +++ b/tools/cpplint.py @@ -1,4 +1,4 @@ -#!/usr/bin/python2.4 +#!/usr/bin/env python # # Copyright (c) 2009 Google Inc. All rights reserved. # @@ -28,43 +28,8 @@ # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -# Here are some issues that I've had people identify in my code during reviews, -# that I think are possible to flag automatically in a lint tool. If these were -# caught by lint, it would save time both for myself and that of my reviewers. -# Most likely, some of these are beyond the scope of the current lint framework, -# but I think it is valuable to retain these wish-list items even if they cannot -# be immediately implemented. -# -# Suggestions -# ----------- -# - Check for no 'explicit' for multi-arg ctor -# - Check for boolean assign RHS in parens -# - Check for ctor initializer-list colon position and spacing -# - Check that if there's a ctor, there should be a dtor -# - Check accessors that return non-pointer member variables are -# declared const -# - Check accessors that return non-const pointer member vars are -# *not* declared const -# - Check for using public includes for testing -# - Check for spaces between brackets in one-line inline method -# - Check for no assert() -# - Check for spaces surrounding operators -# - Check for 0 in pointer context (should be NULL) -# - Check for 0 in char context (should be '\0') -# - Check for camel-case method name conventions for methods -# that are not simple inline getters and setters -# - Check that base classes have virtual destructors -# put " // namespace" after } that closes a namespace, with -# namespace's name after 'namespace' if it is named. -# - Do not indent namespace contents -# - Avoid inlining non-trivial constructors in header files -# include base/basictypes.h if DISALLOW_EVIL_CONSTRUCTORS is used -# - Check for old-school (void) cast for call-sites of functions -# ignored return value -# - Check gUnit usage of anonymous namespace -# - Check for class declaration order (typedefs, consts, enums, -# ctor(s?), dtor, friend declarations, methods, member vars) -# +# Imported from https://github.com/cpplint/cpplint: +# https://github.com/cpplint/cpplint/blob/0f2319741f3407d8638cdc7c41e4fc9bad217f68/cpplint.py """Does google-lint on c++ files. @@ -80,23 +45,51 @@ """ import codecs +import copy import getopt +import glob +import itertools import math # for log import os import re import sre_compile import string import sys +import sysconfig import unicodedata +import xml.etree.ElementTree + +# if empty, use defaults +_valid_extensions = set([]) + +__VERSION__ = '1.5.4' + +try: + xrange # Python 2 +except NameError: + # -- pylint: disable=redefined-builtin + xrange = range # Python 3 _USAGE = """ -Syntax: cpplint.py [--verbose=#] [--output=vs7] [--filter=-x,+y,...] - [--counting=total|toplevel|detailed] +Syntax: cpplint.py [--verbose=#] [--output=emacs|eclipse|vs7|junit|sed|gsed] + [--filter=-x,+y,...] + [--counting=total|toplevel|detailed] [--root=subdir] + [--repository=path] + [--linelength=digits] [--headers=x,y,...] + [--recursive] + [--exclude=path] + [--extensions=hpp,cpp,...] + [--includeorder=default|standardcfirst] + [--quiet] + [--version] [file] ... + Style checker for C/C++ source files. + This is a fork of the Google style checker with minor extensions. + The style guidelines this tries to follow are those in - http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml + https://google.github.io/styleguide/cppguide.html Every problem is given a confidence score from 1-5, with 5 meaning we are certain of the problem, and 1 meaning it could be a legitimate construct. @@ -107,16 +100,30 @@ suppresses errors of all categories on that line. The files passed in will be linted; at least one file must be provided. - Linted extensions are .cc, .cpp, and .h. Other file types will be ignored. + Default linted extensions are %s. + Other file types will be ignored. + Change the extensions with the --extensions flag. Flags: - output=vs7 + output=emacs|eclipse|vs7|junit|sed|gsed By default, the output is formatted to ease emacs parsing. Visual Studio - compatible output (vs7) may also be used. Other formats are unsupported. + compatible output (vs7) may also be used. Further support exists for + eclipse (eclipse), and JUnit (junit). XML parsers such as those used + in Jenkins and Bamboo may also be used. + The sed format outputs sed commands that should fix some of the errors. + Note that this requires gnu sed. If that is installed as gsed on your + system (common e.g. on macOS with homebrew) you can use the gsed output + format. Sed commands are written to stdout, not stderr, so you should be + able to pipe output straight to a shell to run the fixes. verbose=# Specify a number 0-5 to restrict errors to certain verbosity levels. + Errors with lower verbosity levels have lower confidence and are more + likely to be false positives. + + quiet + Don't print anything if no errors are found. filter=-x,+y,... Specify a comma-separated list of category-filters to apply: only @@ -139,134 +146,634 @@ the top-level categories like 'build' and 'whitespace' will also be printed. If 'detailed' is provided, then a count is provided for each category like 'build/class'. + + repository=path + The top level directory of the repository, used to derive the header + guard CPP variable. By default, this is determined by searching for a + path that contains .git, .hg, or .svn. When this flag is specified, the + given path is used instead. This option allows the header guard CPP + variable to remain consistent even if members of a team have different + repository root directories (such as when checking out a subdirectory + with SVN). In addition, users of non-mainstream version control systems + can use this flag to ensure readable header guard CPP variables. + + Examples: + Assuming that Alice checks out ProjectName and Bob checks out + ProjectName/trunk and trunk contains src/chrome/ui/browser.h, then + with no --repository flag, the header guard CPP variable will be: + + Alice => TRUNK_SRC_CHROME_BROWSER_UI_BROWSER_H_ + Bob => SRC_CHROME_BROWSER_UI_BROWSER_H_ + + If Alice uses the --repository=trunk flag and Bob omits the flag or + uses --repository=. then the header guard CPP variable will be: + + Alice => SRC_CHROME_BROWSER_UI_BROWSER_H_ + Bob => SRC_CHROME_BROWSER_UI_BROWSER_H_ + + root=subdir + The root directory used for deriving header guard CPP variable. + This directory is relative to the top level directory of the repository + which by default is determined by searching for a directory that contains + .git, .hg, or .svn but can also be controlled with the --repository flag. + If the specified directory does not exist, this flag is ignored. + + Examples: + Assuming that src is the top level directory of the repository (and + cwd=top/src), the header guard CPP variables for + src/chrome/browser/ui/browser.h are: + + No flag => CHROME_BROWSER_UI_BROWSER_H_ + --root=chrome => BROWSER_UI_BROWSER_H_ + --root=chrome/browser => UI_BROWSER_H_ + --root=.. => SRC_CHROME_BROWSER_UI_BROWSER_H_ + + linelength=digits + This is the allowed line length for the project. The default value is + 80 characters. + + Examples: + --linelength=120 + + recursive + Search for files to lint recursively. Each directory given in the list + of files to be linted is replaced by all files that descend from that + directory. Files with extensions not in the valid extensions list are + excluded. + + exclude=path + Exclude the given path from the list of files to be linted. Relative + paths are evaluated relative to the current directory and shell globbing + is performed. This flag can be provided multiple times to exclude + multiple files. + + Examples: + --exclude=one.cc + --exclude=src/*.cc + --exclude=src/*.cc --exclude=test/*.cc + + extensions=extension,extension,... + The allowed file extensions that cpplint will check + + Examples: + --extensions=%s + + includeorder=default|standardcfirst + For the build/include_order rule, the default is to blindly assume angle + bracket includes with file extension are c-system-headers (default), + even knowing this will have false classifications. + The default is established at google. + standardcfirst means to instead use an allow-list of known c headers and + treat all others as separate group of "other system headers". The C headers + included are those of the C-standard lib and closely related ones. + + headers=x,y,... + The header extensions that cpplint will treat as .h in checks. Values are + automatically added to --extensions list. + (by default, only files with extensions %s will be assumed to be headers) + + Examples: + --headers=%s + --headers=hpp,hxx + --headers=hpp + + cpplint.py supports per-directory configurations specified in CPPLINT.cfg + files. CPPLINT.cfg file can contain a number of key=value pairs. + Currently the following options are supported: + + set noparent + filter=+filter1,-filter2,... + exclude_files=regex + linelength=80 + root=subdir + headers=x,y,... + + "set noparent" option prevents cpplint from traversing directory tree + upwards looking for more .cfg files in parent directories. This option + is usually placed in the top-level project directory. + + The "filter" option is similar in function to --filter flag. It specifies + message filters in addition to the |_DEFAULT_FILTERS| and those specified + through --filter command-line flag. + + "exclude_files" allows to specify a regular expression to be matched against + a file name. If the expression matches, the file is skipped and not run + through the linter. + + "linelength" allows to specify the allowed line length for the project. + + The "root" option is similar in function to the --root flag (see example + above). Paths are relative to the directory of the CPPLINT.cfg. + + The "headers" option is similar in function to the --headers flag + (see example above). + + CPPLINT.cfg has an effect on files in the same directory and all + sub-directories, unless overridden by a nested configuration file. + + Example file: + filter=-build/include_order,+build/include_alpha + exclude_files=.*\\.cc + + The above example disables build/include_order warning and enables + build/include_alpha as well as excludes all .cc from being + processed by linter, in the current directory (where the .cfg + file is located) and all sub-directories. """ # We categorize each error message we print. Here are the categories. # We want an explicit list so we can list them all in cpplint --filter=. # If you add a new error message with a new category, add it to the list # here! cpplint_unittest.py should tell you if you forget to do this. -# \ used for clearer layout -- pylint: disable-msg=C6013 _ERROR_CATEGORIES = [ - 'build/class', - 'build/deprecated', - 'build/endif_comment', - 'build/explicit_make_pair', - 'build/forward_decl', - 'build/header_guard', - 'build/include', - 'build/include_alpha', - 'build/include_order', - 'build/include_what_you_use', - 'build/namespaces', - 'build/printf_format', - 'build/storage_class', - 'legal/copyright', - 'readability/braces', - 'readability/casting', - 'readability/check', - 'readability/constructors', - 'readability/fn_size', - 'readability/function', - 'readability/multiline_comment', - 'readability/multiline_string', - 'readability/nolint', - 'readability/streams', - 'readability/todo', - 'readability/utf8', - 'runtime/arrays', - 'runtime/casting', - 'runtime/explicit', - 'runtime/int', - 'runtime/init', - 'runtime/invalid_increment', - 'runtime/member_string_references', - 'runtime/memset', - 'runtime/operator', - 'runtime/printf', - 'runtime/printf_format', - 'runtime/references', - 'runtime/rtti', - 'runtime/sizeof', - 'runtime/string', - 'runtime/threadsafe_fn', - 'runtime/virtual', - 'whitespace/blank_line', - 'whitespace/braces', - 'whitespace/comma', - 'whitespace/comments', - 'whitespace/end_of_line', - 'whitespace/ending_newline', - 'whitespace/indent', - 'whitespace/labels', - 'whitespace/line_length', - 'whitespace/newline', - 'whitespace/operators', - 'whitespace/parens', - 'whitespace/semicolon', - 'whitespace/tab', - 'whitespace/todo' - ] + 'build/class', + 'build/c++11', + 'build/c++14', + 'build/c++tr1', + 'build/deprecated', + 'build/endif_comment', + 'build/explicit_make_pair', + 'build/forward_decl', + 'build/header_guard', + 'build/include', + 'build/include_subdir', + 'build/include_alpha', + 'build/include_order', + 'build/include_what_you_use', + 'build/namespaces_headers', + 'build/namespaces_literals', + 'build/namespaces', + 'build/printf_format', + 'build/storage_class', + 'legal/copyright', + 'readability/alt_tokens', + 'readability/braces', + 'readability/casting', + 'readability/check', + 'readability/constructors', + 'readability/fn_size', + 'readability/inheritance', + 'readability/multiline_comment', + 'readability/multiline_string', + 'readability/namespace', + 'readability/nolint', + 'readability/nul', + 'readability/strings', + 'readability/todo', + 'readability/utf8', + 'runtime/arrays', + 'runtime/casting', + 'runtime/explicit', + 'runtime/int', + 'runtime/init', + 'runtime/invalid_increment', + 'runtime/member_string_references', + 'runtime/memset', + 'runtime/indentation_namespace', + 'runtime/operator', + 'runtime/printf', + 'runtime/printf_format', + 'runtime/references', + 'runtime/string', + 'runtime/threadsafe_fn', + 'runtime/vlog', + 'whitespace/blank_line', + 'whitespace/braces', + 'whitespace/comma', + 'whitespace/comments', + 'whitespace/empty_conditional_body', + 'whitespace/empty_if_body', + 'whitespace/empty_loop_body', + 'whitespace/end_of_line', + 'whitespace/ending_newline', + 'whitespace/forcolon', + 'whitespace/indent', + 'whitespace/line_length', + 'whitespace/newline', + 'whitespace/operators', + 'whitespace/parens', + 'whitespace/semicolon', + 'whitespace/tab', + 'whitespace/todo', + ] -# These constants define types of headers for use with -# _IncludeState.CheckNextIncludeOrder(). -_C_SYS_HEADER = 1 -_CPP_SYS_HEADER = 2 -_OTHER_HEADER = 3 -_LIKELY_MY_HEADER = 4 -_POSSIBLE_MY_HEADER = 5 +# keywords to use with --outputs which generate stdout for machine processing +_MACHINE_OUTPUTS = [ + 'junit', + 'sed', + 'gsed' +] + +# These error categories are no longer enforced by cpplint, but for backwards- +# compatibility they may still appear in NOLINT comments. +_LEGACY_ERROR_CATEGORIES = [ + 'readability/streams', + 'readability/function', + ] -# The default state of the category filter. This is overrided by the --filter= +# The default state of the category filter. This is overridden by the --filter= # flag. By default all errors are on, so only add here categories that should be # off by default (i.e., categories that must be enabled by the --filter= flags). # All entries here should start with a '-' or '+', as in the --filter= flag. -_DEFAULT_FILTERS = ['-build/include_alpha'] +_DEFAULT_FILTERS = [ + '-build/c++11', + '-build/header_guard', + '-build/include_alpha', + '-build/include_order', + '-build/include_subdir', + '-build/namespaces', + '-readability/namespace', + '-runtime/indentation_namespace', + '-runtime/references', + '-whitespace/blank_line', + '-whitespace/braces', + '-whitespace/indent', + '-whitespace/newline', + '-whitespace/parens', + ] + +# The default list of categories suppressed for C (not C++) files. +_DEFAULT_C_SUPPRESSED_CATEGORIES = [ + 'readability/casting', + ] + +# The default list of categories suppressed for Linux Kernel files. +_DEFAULT_KERNEL_SUPPRESSED_CATEGORIES = [ + 'whitespace/tab', + ] # We used to check for high-bit characters, but after much discussion we # decided those were OK, as long as they were in UTF-8 and didn't represent # hard-coded international strings, which belong in a separate i18n file. -# Headers that we consider STL headers. -_STL_HEADERS = frozenset([ - 'algobase.h', 'algorithm', 'alloc.h', 'bitset', 'deque', 'exception', - 'function.h', 'functional', 'hash_map', 'hash_map.h', 'hash_set', - 'hash_set.h', 'iterator', 'list', 'list.h', 'map', 'memory', 'new', - 'pair.h', 'pthread_alloc', 'queue', 'set', 'set.h', 'sstream', 'stack', - 'stl_alloc.h', 'stl_relops.h', 'type_traits.h', - 'utility', 'vector', 'vector.h', +# C++ headers +_CPP_HEADERS = frozenset([ + # Legacy + 'algobase.h', + 'algo.h', + 'alloc.h', + 'builtinbuf.h', + 'bvector.h', + 'complex.h', + 'defalloc.h', + 'deque.h', + 'editbuf.h', + 'fstream.h', + 'function.h', + 'hash_map', + 'hash_map.h', + 'hash_set', + 'hash_set.h', + 'hashtable.h', + 'heap.h', + 'indstream.h', + 'iomanip.h', + 'iostream.h', + 'istream.h', + 'iterator.h', + 'list.h', + 'map.h', + 'multimap.h', + 'multiset.h', + 'ostream.h', + 'pair.h', + 'parsestream.h', + 'pfstream.h', + 'procbuf.h', + 'pthread_alloc', + 'pthread_alloc.h', + 'rope', + 'rope.h', + 'ropeimpl.h', + 'set.h', + 'slist', + 'slist.h', + 'stack.h', + 'stdiostream.h', + 'stl_alloc.h', + 'stl_relops.h', + 'streambuf.h', + 'stream.h', + 'strfile.h', + 'strstream.h', + 'tempbuf.h', + 'tree.h', + 'type_traits.h', + 'vector.h', + # 17.6.1.2 C++ library headers + 'algorithm', + 'array', + 'atomic', + 'bitset', + 'chrono', + 'codecvt', + 'complex', + 'condition_variable', + 'deque', + 'exception', + 'forward_list', + 'fstream', + 'functional', + 'future', + 'initializer_list', + 'iomanip', + 'ios', + 'iosfwd', + 'iostream', + 'istream', + 'iterator', + 'limits', + 'list', + 'locale', + 'map', + 'memory', + 'mutex', + 'new', + 'numeric', + 'ostream', + 'queue', + 'random', + 'ratio', + 'regex', + 'scoped_allocator', + 'set', + 'sstream', + 'stack', + 'stdexcept', + 'streambuf', + 'string', + 'strstream', + 'system_error', + 'thread', + 'tuple', + 'typeindex', + 'typeinfo', + 'type_traits', + 'unordered_map', + 'unordered_set', + 'utility', + 'valarray', + 'vector', + # 17.6.1.2 C++14 headers + 'shared_mutex', + # 17.6.1.2 C++17 headers + 'any', + 'charconv', + 'codecvt', + 'execution', + 'filesystem', + 'memory_resource', + 'optional', + 'string_view', + 'variant', + # 17.6.1.2 C++ headers for C library facilities + 'cassert', + 'ccomplex', + 'cctype', + 'cerrno', + 'cfenv', + 'cfloat', + 'cinttypes', + 'ciso646', + 'climits', + 'clocale', + 'cmath', + 'csetjmp', + 'csignal', + 'cstdalign', + 'cstdarg', + 'cstdbool', + 'cstddef', + 'cstdint', + 'cstdio', + 'cstdlib', + 'cstring', + 'ctgmath', + 'ctime', + 'cuchar', + 'cwchar', + 'cwctype', ]) - -# Non-STL C++ system headers. -_CPP_HEADERS = frozenset([ - 'algo.h', 'builtinbuf.h', 'bvector.h', 'cassert', 'cctype', - 'cerrno', 'cfloat', 'ciso646', 'climits', 'clocale', 'cmath', - 'complex', 'complex.h', 'csetjmp', 'csignal', 'cstdarg', 'cstddef', - 'cstdio', 'cstdlib', 'cstring', 'ctime', 'cwchar', 'cwctype', - 'defalloc.h', 'deque.h', 'editbuf.h', 'exception', 'fstream', - 'fstream.h', 'hashtable.h', 'heap.h', 'indstream.h', 'iomanip', - 'iomanip.h', 'ios', 'iosfwd', 'iostream', 'iostream.h', 'istream', - 'istream.h', 'iterator.h', 'limits', 'map.h', 'multimap.h', 'multiset.h', - 'numeric', 'ostream', 'ostream.h', 'parsestream.h', 'pfstream.h', - 'PlotFile.h', 'procbuf.h', 'pthread_alloc.h', 'rope', 'rope.h', - 'ropeimpl.h', 'SFile.h', 'slist', 'slist.h', 'stack.h', 'stdexcept', - 'stdiostream.h', 'streambuf.h', 'stream.h', 'strfile.h', 'string', - 'strstream', 'strstream.h', 'tempbuf.h', 'tree.h', 'typeinfo', 'valarray', +# C headers +_C_HEADERS = frozenset([ + # System C headers + 'assert.h', + 'complex.h', + 'ctype.h', + 'errno.h', + 'fenv.h', + 'float.h', + 'inttypes.h', + 'iso646.h', + 'limits.h', + 'locale.h', + 'math.h', + 'setjmp.h', + 'signal.h', + 'stdalign.h', + 'stdarg.h', + 'stdatomic.h', + 'stdbool.h', + 'stddef.h', + 'stdint.h', + 'stdio.h', + 'stdlib.h', + 'stdnoreturn.h', + 'string.h', + 'tgmath.h', + 'threads.h', + 'time.h', + 'uchar.h', + 'wchar.h', + 'wctype.h', + # additional POSIX C headers + 'aio.h', + 'arpa/inet.h', + 'cpio.h', + 'dirent.h', + 'dlfcn.h', + 'fcntl.h', + 'fmtmsg.h', + 'fnmatch.h', + 'ftw.h', + 'glob.h', + 'grp.h', + 'iconv.h', + 'langinfo.h', + 'libgen.h', + 'monetary.h', + 'mqueue.h', + 'ndbm.h', + 'net/if.h', + 'netdb.h', + 'netinet/in.h', + 'netinet/tcp.h', + 'nl_types.h', + 'poll.h', + 'pthread.h', + 'pwd.h', + 'regex.h', + 'sched.h', + 'search.h', + 'semaphore.h', + 'setjmp.h', + 'signal.h', + 'spawn.h', + 'strings.h', + 'stropts.h', + 'syslog.h', + 'tar.h', + 'termios.h', + 'trace.h', + 'ulimit.h', + 'unistd.h', + 'utime.h', + 'utmpx.h', + 'wordexp.h', + # additional GNUlib headers + 'a.out.h', + 'aliases.h', + 'alloca.h', + 'ar.h', + 'argp.h', + 'argz.h', + 'byteswap.h', + 'crypt.h', + 'endian.h', + 'envz.h', + 'err.h', + 'error.h', + 'execinfo.h', + 'fpu_control.h', + 'fstab.h', + 'fts.h', + 'getopt.h', + 'gshadow.h', + 'ieee754.h', + 'ifaddrs.h', + 'libintl.h', + 'mcheck.h', + 'mntent.h', + 'obstack.h', + 'paths.h', + 'printf.h', + 'pty.h', + 'resolv.h', + 'shadow.h', + 'sysexits.h', + 'ttyent.h', + # Additional linux glibc headers + 'dlfcn.h', + 'elf.h', + 'features.h', + 'gconv.h', + 'gnu-versions.h', + 'lastlog.h', + 'libio.h', + 'link.h', + 'malloc.h', + 'memory.h', + 'netash/ash.h', + 'netatalk/at.h', + 'netax25/ax25.h', + 'neteconet/ec.h', + 'netipx/ipx.h', + 'netiucv/iucv.h', + 'netpacket/packet.h', + 'netrom/netrom.h', + 'netrose/rose.h', + 'nfs/nfs.h', + 'nl_types.h', + 'nss.h', + 're_comp.h', + 'regexp.h', + 'sched.h', + 'sgtty.h', + 'stab.h', + 'stdc-predef.h', + 'stdio_ext.h', + 'syscall.h', + 'termio.h', + 'thread_db.h', + 'ucontext.h', + 'ustat.h', + 'utmp.h', + 'values.h', + 'wait.h', + 'xlocale.h', + # Hardware specific headers + 'arm_neon.h', + 'emmintrin.h', + 'xmmintin.h', ]) +# Folders of C libraries so commonly used in C++, +# that they have parity with standard C libraries. +C_STANDARD_HEADER_FOLDERS = frozenset([ + # standard C library + "sys", + # glibc for linux + "arpa", + "asm-generic", + "bits", + "gnu", + "net", + "netinet", + "protocols", + "rpc", + "rpcsvc", + "scsi", + # linux kernel header + "drm", + "linux", + "misc", + "mtd", + "rdma", + "sound", + "video", + "xen", + ]) + +# Type names +_TYPES = re.compile( + r'^(?:' + # [dcl.type.simple] + r'(char(16_t|32_t)?)|wchar_t|' + r'bool|short|int|long|signed|unsigned|float|double|' + # [support.types] + r'(ptrdiff_t|size_t|max_align_t|nullptr_t)|' + # [cstdint.syn] + r'(u?int(_fast|_least)?(8|16|32|64)_t)|' + r'(u?int(max|ptr)_t)|' + r')$') + + +# These headers are excluded from [build/include] and [build/include_order] +# checks: +# - Anything not following google file name conventions (containing an +# uppercase character, such as Python.h or nsStringAPI.h, for example). +# - Lua headers. +_THIRD_PARTY_HEADERS_PATTERN = re.compile( + r'^(?:[^/]*[A-Z][^/]*\.h|lua\.h|lauxlib\.h|lualib\.h)$') + +# Pattern for matching FileInfo.BaseName() against test file name +_test_suffixes = ['_test', '_regtest', '_unittest'] +_TEST_FILE_SUFFIX = '(' + '|'.join(_test_suffixes) + r')$' + +# Pattern that matches only complete whitespace, possibly across multiple lines. +_EMPTY_CONDITIONAL_BODY_PATTERN = re.compile(r'^\s*$', re.DOTALL) # Assertion macros. These are defined in base/logging.h and -# testing/base/gunit.h. Note that the _M versions need to come first -# for substring matching to work. +# testing/base/public/gunit.h. _CHECK_MACROS = [ 'DCHECK', 'CHECK', - 'EXPECT_TRUE_M', 'EXPECT_TRUE', - 'ASSERT_TRUE_M', 'ASSERT_TRUE', - 'EXPECT_FALSE_M', 'EXPECT_FALSE', - 'ASSERT_FALSE_M', 'ASSERT_FALSE', + 'EXPECT_TRUE', 'ASSERT_TRUE', + 'EXPECT_FALSE', 'ASSERT_FALSE', ] # Replacement macros for CHECK/DCHECK/EXPECT_TRUE/EXPECT_FALSE -_CHECK_REPLACEMENT = dict([(m, {}) for m in _CHECK_MACROS]) +_CHECK_REPLACEMENT = dict([(macro_var, {}) for macro_var in _CHECK_MACROS]) for op, replacement in [('==', 'EQ'), ('!=', 'NE'), ('>=', 'GE'), ('>', 'GT'), @@ -275,29 +782,196 @@ _CHECK_REPLACEMENT['CHECK'][op] = 'CHECK_%s' % replacement _CHECK_REPLACEMENT['EXPECT_TRUE'][op] = 'EXPECT_%s' % replacement _CHECK_REPLACEMENT['ASSERT_TRUE'][op] = 'ASSERT_%s' % replacement - _CHECK_REPLACEMENT['EXPECT_TRUE_M'][op] = 'EXPECT_%s_M' % replacement - _CHECK_REPLACEMENT['ASSERT_TRUE_M'][op] = 'ASSERT_%s_M' % replacement for op, inv_replacement in [('==', 'NE'), ('!=', 'EQ'), ('>=', 'LT'), ('>', 'LE'), ('<=', 'GT'), ('<', 'GE')]: _CHECK_REPLACEMENT['EXPECT_FALSE'][op] = 'EXPECT_%s' % inv_replacement _CHECK_REPLACEMENT['ASSERT_FALSE'][op] = 'ASSERT_%s' % inv_replacement - _CHECK_REPLACEMENT['EXPECT_FALSE_M'][op] = 'EXPECT_%s_M' % inv_replacement - _CHECK_REPLACEMENT['ASSERT_FALSE_M'][op] = 'ASSERT_%s_M' % inv_replacement + +# Alternative tokens and their replacements. For full list, see section 2.5 +# Alternative tokens [lex.digraph] in the C++ standard. +# +# Digraphs (such as '%:') are not included here since it's a mess to +# match those on a word boundary. +_ALT_TOKEN_REPLACEMENT = { + 'and': '&&', + 'bitor': '|', + 'or': '||', + 'xor': '^', + 'compl': '~', + 'bitand': '&', + 'and_eq': '&=', + 'or_eq': '|=', + 'xor_eq': '^=', + 'not': '!', + 'not_eq': '!=' + } + +# Compile regular expression that matches all the above keywords. The "[ =()]" +# bit is meant to avoid matching these keywords outside of boolean expressions. +# +# False positives include C-style multi-line comments and multi-line strings +# but those have always been troublesome for cpplint. +_ALT_TOKEN_REPLACEMENT_PATTERN = re.compile( + r'[ =()](' + ('|'.join(_ALT_TOKEN_REPLACEMENT.keys())) + r')(?=[ (]|$)') -_regexp_compile_cache = {} +# These constants define types of headers for use with +# _IncludeState.CheckNextIncludeOrder(). +_C_SYS_HEADER = 1 +_CPP_SYS_HEADER = 2 +_OTHER_SYS_HEADER = 3 +_LIKELY_MY_HEADER = 4 +_POSSIBLE_MY_HEADER = 5 +_OTHER_HEADER = 6 + +# These constants define the current inline assembly state +_NO_ASM = 0 # Outside of inline assembly block +_INSIDE_ASM = 1 # Inside inline assembly block +_END_ASM = 2 # Last line of inline assembly block +_BLOCK_ASM = 3 # The whole block is an inline assembly block + +# Match start of assembly blocks +_MATCH_ASM = re.compile(r'^\s*(?:asm|_asm|__asm|__asm__)' + r'(?:\s+(volatile|__volatile__))?' + r'\s*[{(]') + +# Match strings that indicate we're working on a C (not C++) file. +_SEARCH_C_FILE = re.compile(r'\b(?:LINT_C_FILE|' + r'vim?:\s*.*(\s*|:)filetype=c(\s*|:|$))') + +# Match string that indicates we're working on a Linux Kernel file. +_SEARCH_KERNEL_FILE = re.compile(r'\b(?:LINT_KERNEL_FILE)') + +# Commands for sed to fix the problem +_SED_FIXUPS = { + 'Remove spaces around =': r's/ = /=/', + 'Remove spaces around !=': r's/ != /!=/', + 'Remove space before ( in if (': r's/if (/if(/', + 'Remove space before ( in for (': r's/for (/for(/', + 'Remove space before ( in while (': r's/while (/while(/', + 'Remove space before ( in switch (': r's/switch (/switch(/', + 'Should have a space between // and comment': r's/\/\//\/\/ /', + 'Missing space before {': r's/\([^ ]\){/\1 {/', + 'Tab found, replace by spaces': r's/\t/ /g', + 'Line ends in whitespace. Consider deleting these extra spaces.': r's/\s*$//', + 'You don\'t need a ; after a }': r's/};/}/', + 'Missing space after ,': r's/,\([^ ]\)/, \1/g', +} -# Finds occurrences of NOLINT or NOLINT(...). -_RE_SUPPRESSION = re.compile(r'\bNOLINT\b(\([^)]*\))?') +_regexp_compile_cache = {} # {str, set(int)}: a map from error categories to sets of linenumbers # on which those errors are expected and should be suppressed. _error_suppressions = {} +# The root directory used for deriving header guard CPP variable. +# This is set by --root flag. +_root = None +_root_debug = False + +# The top level repository directory. If set, _root is calculated relative to +# this directory instead of the directory containing version control artifacts. +# This is set by the --repository flag. +_repository = None + +# Files to exclude from linting. This is set by the --exclude flag. +_excludes = None + +# Whether to supress all PrintInfo messages, UNRELATED to --quiet flag +_quiet = False + +# The allowed line length of files. +# This is set by --linelength flag. +_line_length = 80 + +# This allows to use different include order rule than default +_include_order = "default" + +try: + unicode +except NameError: + # -- pylint: disable=redefined-builtin + basestring = unicode = str + +try: + long +except NameError: + # -- pylint: disable=redefined-builtin + long = int + +if sys.version_info < (3,): + # -- pylint: disable=no-member + # BINARY_TYPE = str + itervalues = dict.itervalues + iteritems = dict.iteritems +else: + # BINARY_TYPE = bytes + itervalues = dict.values + iteritems = dict.items + +def unicode_escape_decode(x): + if sys.version_info < (3,): + return codecs.unicode_escape_decode(x)[0] + else: + return x + +# Treat all headers starting with 'h' equally: .h, .hpp, .hxx etc. +# This is set by --headers flag. +_hpp_headers = set([]) + +# {str, bool}: a map from error categories to booleans which indicate if the +# category should be suppressed for every line. +_global_error_suppressions = {} + +def ProcessHppHeadersOption(val): + global _hpp_headers + try: + _hpp_headers = {ext.strip() for ext in val.split(',')} + except ValueError: + PrintUsage('Header extensions must be comma separated list.') + +def ProcessIncludeOrderOption(val): + if val is None or val == "default": + pass + elif val == "standardcfirst": + global _include_order + _include_order = val + else: + PrintUsage('Invalid includeorder value %s. Expected default|standardcfirst') + +def IsHeaderExtension(file_extension): + return file_extension in GetHeaderExtensions() + +def GetHeaderExtensions(): + if _hpp_headers: + return _hpp_headers + if _valid_extensions: + return {h for h in _valid_extensions if 'h' in h} + return set(['h', 'hh', 'hpp', 'hxx', 'h++', 'cuh']) + +# The allowed extensions for file names +# This is set by --extensions flag +def GetAllExtensions(): + return GetHeaderExtensions().union(_valid_extensions or set( + ['c', 'cc', 'cpp', 'cxx', 'c++', 'cu'])) + +def ProcessExtensionsOption(val): + global _valid_extensions + try: + extensions = [ext.strip() for ext in val.split(',')] + _valid_extensions = set(extensions) + except ValueError: + PrintUsage('Extensions should be a comma-separated list of values;' + 'for example: extensions=hpp,cpp\n' + 'This could not be parsed: "%s"' % (val,)) + +def GetNonHeaderExtensions(): + return GetAllExtensions().difference(GetHeaderExtensions()) + def ParseNolintSuppressions(filename, raw_line, linenum, error): - """Updates the global list of error-suppressions. + """Updates the global list of line error-suppressions. Parses any NOLINT comments on the current line, updating the global error_suppressions store. Reports an error if the NOLINT comment @@ -309,64 +983,113 @@ def ParseNolintSuppressions(filename, raw_line, linenum, error): linenum: int, the number of the current line. error: function, an error handler. """ - # FIXME(adonovan): "NOLINT(" is misparsed as NOLINT(*). - matched = _RE_SUPPRESSION.search(raw_line) + matched = Search(r'\bNOLINT(NEXTLINE)?\b(\([^)]+\))?', raw_line) if matched: - category = matched.group(1) + if matched.group(1): + suppressed_line = linenum + 1 + else: + suppressed_line = linenum + category = matched.group(2) if category in (None, '(*)'): # => "suppress all" - _error_suppressions.setdefault(None, set()).add(linenum) + _error_suppressions.setdefault(None, set()).add(suppressed_line) else: if category.startswith('(') and category.endswith(')'): category = category[1:-1] if category in _ERROR_CATEGORIES: - _error_suppressions.setdefault(category, set()).add(linenum) - else: + _error_suppressions.setdefault(category, set()).add(suppressed_line) + elif category not in _LEGACY_ERROR_CATEGORIES: error(filename, linenum, 'readability/nolint', 5, 'Unknown NOLINT error category: %s' % category) +def ProcessGlobalSuppresions(lines): + """Updates the list of global error suppressions. + + Parses any lint directives in the file that have global effect. + + Args: + lines: An array of strings, each representing a line of the file, with the + last element being empty if the file is terminated with a newline. + """ + for line in lines: + if _SEARCH_C_FILE.search(line): + for category in _DEFAULT_C_SUPPRESSED_CATEGORIES: + _global_error_suppressions[category] = True + if _SEARCH_KERNEL_FILE.search(line): + for category in _DEFAULT_KERNEL_SUPPRESSED_CATEGORIES: + _global_error_suppressions[category] = True + + def ResetNolintSuppressions(): - "Resets the set of NOLINT suppressions to empty." + """Resets the set of NOLINT suppressions to empty.""" _error_suppressions.clear() + _global_error_suppressions.clear() def IsErrorSuppressedByNolint(category, linenum): """Returns true if the specified error category is suppressed on this line. Consults the global error_suppressions map populated by - ParseNolintSuppressions/ResetNolintSuppressions. + ParseNolintSuppressions/ProcessGlobalSuppresions/ResetNolintSuppressions. Args: category: str, the category of the error. linenum: int, the current line number. Returns: - bool, True iff the error should be suppressed due to a NOLINT comment. + bool, True iff the error should be suppressed due to a NOLINT comment or + global suppression. """ - return (linenum in _error_suppressions.get(category, set()) or + return (_global_error_suppressions.get(category, False) or + linenum in _error_suppressions.get(category, set()) or linenum in _error_suppressions.get(None, set())) + def Match(pattern, s): """Matches the string with the pattern, caching the compiled regexp.""" # The regexp compilation caching is inlined in both Match and Search for # performance reasons; factoring it out into a separate function turns out # to be noticeably expensive. - if not pattern in _regexp_compile_cache: + if pattern not in _regexp_compile_cache: _regexp_compile_cache[pattern] = sre_compile.compile(pattern) return _regexp_compile_cache[pattern].match(s) +def ReplaceAll(pattern, rep, s): + """Replaces instances of pattern in a string with a replacement. + + The compiled regex is kept in a cache shared by Match and Search. + + Args: + pattern: regex pattern + rep: replacement text + s: search string + + Returns: + string with replacements made (or original string if no replacements) + """ + if pattern not in _regexp_compile_cache: + _regexp_compile_cache[pattern] = sre_compile.compile(pattern) + return _regexp_compile_cache[pattern].sub(rep, s) + + def Search(pattern, s): """Searches the string for the pattern, caching the compiled regexp.""" - if not pattern in _regexp_compile_cache: + if pattern not in _regexp_compile_cache: _regexp_compile_cache[pattern] = sre_compile.compile(pattern) return _regexp_compile_cache[pattern].search(s) -class _IncludeState(dict): +def _IsSourceExtension(s): + """File extension (excluding dot) matches a source file extension.""" + return s in GetNonHeaderExtensions() + + +class _IncludeState(object): """Tracks line numbers for includes, and the order in which includes appear. - As a dict, an _IncludeState object serves as a mapping between include - filename and line number on which that file was included. + include_list contains list of lists of (header, line number) pairs. + It's a lists of lists rather than just one flat list to make it + easier to update across preprocessor boundaries. Call CheckNextIncludeOrder() once for each header in the file, passing in the type constants defined above. Calls in an illegal order will @@ -376,33 +1099,71 @@ class _IncludeState(dict): # self._section will move monotonically through this set. If it ever # needs to move backwards, CheckNextIncludeOrder will raise an error. _INITIAL_SECTION = 0 - _C_SECTION = 1 - _CPP_SECTION = 2 - _OTHER_H_SECTION = 3 - _MY_H_SECTION = 4 + _MY_H_SECTION = 1 + _C_SECTION = 2 + _CPP_SECTION = 3 + _OTHER_SYS_SECTION = 4 + _OTHER_H_SECTION = 5 _TYPE_NAMES = { _C_SYS_HEADER: 'C system header', _CPP_SYS_HEADER: 'C++ system header', + _OTHER_SYS_HEADER: 'other system header', _LIKELY_MY_HEADER: 'header this file implements', _POSSIBLE_MY_HEADER: 'header this file may implement', _OTHER_HEADER: 'other header', } _SECTION_NAMES = { _INITIAL_SECTION: "... nothing. (This can't be an error.)", + _MY_H_SECTION: 'a header this file implements', _C_SECTION: 'C system header', _CPP_SECTION: 'C++ system header', + _OTHER_SYS_SECTION: 'other system header', _OTHER_H_SECTION: 'other header', - _MY_H_SECTION: 'a header this file implements', } def __init__(self): - dict.__init__(self) + self.include_list = [[]] + self._section = None + self._last_header = None + self.ResetSection('') + + def FindHeader(self, header): + """Check if a header has already been included. + + Args: + header: header to check. + Returns: + Line number of previous occurrence, or -1 if the header has not + been seen before. + """ + for section_list in self.include_list: + for f in section_list: + if f[0] == header: + return f[1] + return -1 + + def ResetSection(self, directive): + """Reset section checking for preprocessor directive. + + Args: + directive: preprocessor directive (e.g. "if", "else"). + """ # The name of the current section. self._section = self._INITIAL_SECTION # The path of last found header. self._last_header = '' + # Update list of includes. Note that we never pop from the + # include list. + if directive in ('if', 'ifdef', 'ifndef'): + self.include_list.append([]) + elif directive in ('else', 'elif'): + self.include_list[-1] = [] + + def SetLastHeader(self, header_path): + self._last_header = header_path + def CanonicalizeAlphabeticalOrder(self, header_path): """Returns a path canonicalized for alphabetical comparison. @@ -418,19 +1179,25 @@ def CanonicalizeAlphabeticalOrder(self, header_path): """ return header_path.replace('-inl.h', '.h').replace('-', '_').lower() - def IsInAlphabeticalOrder(self, header_path): + def IsInAlphabeticalOrder(self, clean_lines, linenum, header_path): """Check if a header is in alphabetical order with the previous header. Args: - header_path: Header to be checked. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + header_path: Canonicalized header to be checked. Returns: Returns true if the header is in alphabetical order. """ - canonical_header = self.CanonicalizeAlphabeticalOrder(header_path) - if self._last_header > canonical_header: + # If previous section is different from current section, _last_header will + # be reset to empty string, so it's always less than current header. + # + # If previous line was a blank line, assume that the headers are + # intentionally sorted the way they are. + if (self._last_header > header_path and + Match(r'^\s*#\s*include\b', clean_lines.elided[linenum - 1])): return False - self._last_header = canonical_header return True def CheckNextIncludeOrder(self, header_type): @@ -465,6 +1232,12 @@ def CheckNextIncludeOrder(self, header_type): else: self._last_header = '' return error_message + elif header_type == _OTHER_SYS_HEADER: + if self._section <= self._OTHER_SYS_SECTION: + self._section = self._OTHER_SYS_SECTION + else: + self._last_header = '' + return error_message elif header_type == _LIKELY_MY_HEADER: if self._section <= self._MY_H_SECTION: self._section = self._MY_H_SECTION @@ -495,18 +1268,36 @@ def __init__(self): self.error_count = 0 # global count of reported errors # filters to apply when emitting error messages self.filters = _DEFAULT_FILTERS[:] + # backup of filter list. Used to restore the state after each file. + self._filters_backup = self.filters[:] self.counting = 'total' # In what way are we counting errors? self.errors_by_category = {} # string to int dict storing error counts + self.quiet = False # Suppress non-error messagess? # output format: # "emacs" - format that emacs can parse (default) + # "eclipse" - format that eclipse can parse # "vs7" - format that Microsoft Visual Studio 7 can parse + # "junit" - format that Jenkins, Bamboo, etc can parse + # "sed" - returns a gnu sed command to fix the problem + # "gsed" - like sed, but names the command gsed, e.g. for macOS homebrew users self.output_format = 'emacs' + # For JUnit output, save errors and failures until the end so that they + # can be written into the XML + self._junit_errors = [] + self._junit_failures = [] + def SetOutputFormat(self, output_format): """Sets the output format for errors.""" self.output_format = output_format + def SetQuiet(self, quiet): + """Sets the module's quiet settings, and returns the previous setting.""" + last_quiet = self.quiet + self.quiet = quiet + return last_quiet + def SetVerboseLevel(self, level): """Sets the module's verbosity, and returns the previous setting.""" last_verbose_level = self.verbose_level @@ -533,6 +1324,10 @@ def SetFilters(self, filters): """ # Default filters always have less priority than the flag ones. self.filters = _DEFAULT_FILTERS[:] + self.AddFilters(filters) + + def AddFilters(self, filters): + """ Adds more filters to the existing list of error-message filters. """ for filt in filters.split(','): clean_filt = filt.strip() if clean_filt: @@ -542,6 +1337,14 @@ def SetFilters(self, filters): raise ValueError('Every filter in --filters must start with + or -' ' (%s does not)' % filt) + def BackupFilters(self): + """ Saves the current filter list to backup storage.""" + self._filters_backup = self.filters[:] + + def RestoreFilters(self): + """ Restores filters previously backed up.""" + self.filters = self._filters_backup[:] + def ResetErrorCounts(self): """Sets the module's error statistic back to zero.""" self.error_count = 0 @@ -559,10 +1362,71 @@ def IncrementErrorCount(self, category): def PrintErrorCounts(self): """Print a summary of errors by category, and the total.""" - for category, count in self.errors_by_category.iteritems(): - sys.stderr.write('Category \'%s\' errors found: %d\n' % + for category, count in sorted(iteritems(self.errors_by_category)): + self.PrintInfo('Category \'%s\' errors found: %d\n' % (category, count)) - sys.stderr.write('Total errors found: %d\n' % self.error_count) + if self.error_count > 0: + self.PrintInfo('Total errors found: %d\n' % self.error_count) + + def PrintInfo(self, message): + # _quiet does not represent --quiet flag. + # Hide infos from stdout to keep stdout pure for machine consumption + if not _quiet and self.output_format not in _MACHINE_OUTPUTS: + sys.stdout.write(message) + + def PrintError(self, message): + if self.output_format == 'junit': + self._junit_errors.append(message) + else: + sys.stderr.write(message) + + def AddJUnitFailure(self, filename, linenum, message, category, confidence): + self._junit_failures.append((filename, linenum, message, category, + confidence)) + + def FormatJUnitXML(self): + num_errors = len(self._junit_errors) + num_failures = len(self._junit_failures) + + testsuite = xml.etree.ElementTree.Element('testsuite') + testsuite.attrib['errors'] = str(num_errors) + testsuite.attrib['failures'] = str(num_failures) + testsuite.attrib['name'] = 'cpplint' + + if num_errors == 0 and num_failures == 0: + testsuite.attrib['tests'] = str(1) + xml.etree.ElementTree.SubElement(testsuite, 'testcase', name='passed') + + else: + testsuite.attrib['tests'] = str(num_errors + num_failures) + if num_errors > 0: + testcase = xml.etree.ElementTree.SubElement(testsuite, 'testcase') + testcase.attrib['name'] = 'errors' + error = xml.etree.ElementTree.SubElement(testcase, 'error') + error.text = '\n'.join(self._junit_errors) + if num_failures > 0: + # Group failures by file + failed_file_order = [] + failures_by_file = {} + for failure in self._junit_failures: + failed_file = failure[0] + if failed_file not in failed_file_order: + failed_file_order.append(failed_file) + failures_by_file[failed_file] = [] + failures_by_file[failed_file].append(failure) + # Create a testcase for each file + for failed_file in failed_file_order: + failures = failures_by_file[failed_file] + testcase = xml.etree.ElementTree.SubElement(testsuite, 'testcase') + testcase.attrib['name'] = failed_file + failure = xml.etree.ElementTree.SubElement(testcase, 'failure') + template = '{0}: {1} [{2}] [{3}]' + texts = [template.format(f[1], f[2], f[3], f[4]) for f in failures] + failure.text = '\n'.join(texts) + + xml_decl = '\n' + return xml_decl + xml.etree.ElementTree.tostring(testsuite, 'utf-8').decode('utf-8') + _cpplint_state = _CppLintState() @@ -576,6 +1440,14 @@ def _SetOutputFormat(output_format): """Sets the module's output format.""" _cpplint_state.SetOutputFormat(output_format) +def _Quiet(): + """Return's the module's quiet setting.""" + return _cpplint_state.quiet + +def _SetQuiet(quiet): + """Set the module's quiet status, and return previous setting.""" + return _cpplint_state.SetQuiet(quiet) + def _VerboseLevel(): """Returns the module's verbosity setting.""" @@ -609,6 +1481,25 @@ def _SetFilters(filters): """ _cpplint_state.SetFilters(filters) +def _AddFilters(filters): + """Adds more filter overrides. + + Unlike _SetFilters, this function does not reset the current list of filters + available. + + Args: + filters: A string of comma-separated filters (eg "whitespace/indent"). + Each filter should start with + or -; else we die. + """ + _cpplint_state.AddFilters(filters) + +def _BackupFilters(): + """ Saves the current filter list to backup storage.""" + _cpplint_state.BackupFilters() + +def _RestoreFilters(): + """ Restores filters previously backed up.""" + _cpplint_state.RestoreFilters() class _FunctionState(object): """Tracks current function name and the number of lines in its body.""" @@ -644,6 +1535,9 @@ def Check(self, error, filename, linenum): filename: The name of the current file. linenum: The number of the line to check. """ + if not self.in_a_function: + return + if Match(r'T(EST|est)', self.current_function): base_trigger = self._TEST_TRIGGER else: @@ -671,7 +1565,7 @@ class _IncludeError(Exception): pass -class FileInfo: +class FileInfo(object): """Provides utility functions for filenames. FileInfo provides easy access to the components of a file's path @@ -686,12 +1580,12 @@ def FullName(self): return os.path.abspath(self._filename).replace('\\', '/') def RepositoryName(self): - """FullName after removing the local path to the repository. + r"""FullName after removing the local path to the repository. If we have a real absolute path name here we can try to do something smart: detecting the root of the checkout and truncating /path/to/checkout from the name so that we get header guards that don't include things like - "C:\Documents and Settings\..." or "/home/username/..." in them and thus + "C:\\Documents and Settings\\..." or "/home/username/..." in them and thus people on different computers who have checked the source out to different locations won't see bogus errors. """ @@ -700,6 +1594,20 @@ def RepositoryName(self): if os.path.exists(fullname): project_dir = os.path.dirname(fullname) + # If the user specified a repository path, it exists, and the file is + # contained in it, use the specified repository path + if _repository: + repo = FileInfo(_repository).FullName() + root_dir = project_dir + while os.path.exists(root_dir): + # allow case insensitive compare on Windows + if os.path.normcase(root_dir) == os.path.normcase(repo): + return os.path.relpath(fullname, root_dir).replace('\\', '/') + one_up_dir = os.path.dirname(root_dir) + if one_up_dir == root_dir: + break + root_dir = one_up_dir + if os.path.exists(os.path.join(project_dir, ".svn")): # If there's a .svn file in the current directory, we recursively look # up the directory tree for the top of the SVN checkout @@ -712,16 +1620,19 @@ def RepositoryName(self): prefix = os.path.commonprefix([root_dir, project_dir]) return fullname[len(prefix) + 1:] - # Not SVN? Try to find a git or hg top level directory by searching up - # from the current path. - root_dir = os.path.dirname(fullname) - while (root_dir != os.path.dirname(root_dir) and - not os.path.exists(os.path.join(root_dir, ".git")) and - not os.path.exists(os.path.join(root_dir, ".hg"))): - root_dir = os.path.dirname(root_dir) + # Not SVN <= 1.6? Try to find a git, hg, or svn top level directory by + # searching up from the current path. + root_dir = current_dir = os.path.dirname(fullname) + while current_dir != os.path.dirname(current_dir): + if (os.path.exists(os.path.join(current_dir, ".git")) or + os.path.exists(os.path.join(current_dir, ".hg")) or + os.path.exists(os.path.join(current_dir, ".svn"))): + root_dir = current_dir + current_dir = os.path.dirname(current_dir) if (os.path.exists(os.path.join(root_dir, ".git")) or - os.path.exists(os.path.join(root_dir, ".hg"))): + os.path.exists(os.path.join(root_dir, ".hg")) or + os.path.exists(os.path.join(root_dir, ".svn"))): prefix = os.path.commonprefix([root_dir, project_dir]) return fullname[len(prefix) + 1:] @@ -747,7 +1658,7 @@ def BaseName(self): return self.Split()[1] def Extension(self): - """File extension - text following the final period.""" + """File extension - text following the final period, includes that period.""" return self.Split()[2] def NoExtension(self): @@ -756,7 +1667,7 @@ def NoExtension(self): def IsSource(self): """File has a source file extension.""" - return self.Extension()[1:] in ('c', 'cc', 'cpp', 'cxx') + return _IsSourceExtension(self.Extension()[1:]) def _ShouldPrintError(category, confidence, linenum): @@ -767,6 +1678,7 @@ def _ShouldPrintError(category, confidence, linenum): # the verbosity level isn't high enough, or the filters filter it out. if IsErrorSuppressedByNolint(category, linenum): return False + if confidence < _cpplint_state.verbose_level: return False @@ -811,21 +1723,32 @@ def Error(filename, linenum, category, confidence, message): if _ShouldPrintError(category, confidence, linenum): _cpplint_state.IncrementErrorCount(category) if _cpplint_state.output_format == 'vs7': - sys.stderr.write('%s(%s): %s [%s] [%d]\n' % ( + _cpplint_state.PrintError('%s(%s): error cpplint: [%s] %s [%d]\n' % ( + filename, linenum, category, message, confidence)) + elif _cpplint_state.output_format == 'eclipse': + sys.stderr.write('%s:%s: warning: %s [%s] [%d]\n' % ( filename, linenum, message, category, confidence)) + elif _cpplint_state.output_format == 'junit': + _cpplint_state.AddJUnitFailure(filename, linenum, message, category, + confidence) + elif _cpplint_state.output_format in ['sed', 'gsed']: + if message in _SED_FIXUPS: + sys.stdout.write(_cpplint_state.output_format + " -i '%s%s' %s # %s [%s] [%d]\n" % ( + linenum, _SED_FIXUPS[message], filename, message, category, confidence)) + else: + sys.stderr.write('# %s:%s: "%s" [%s] [%d]\n' % ( + filename, linenum, message, category, confidence)) else: - sys.stderr.write('%s:%s: %s [%s] [%d]\n' % ( - filename, linenum, message, category, confidence)) + final_message = '%s:%s: %s [%s] [%d]\n' % ( + filename, linenum, message, category, confidence) + sys.stderr.write(final_message) - -# Matches standard C++ escape esequences per 2.13.2.3 of the C++ standard. +# Matches standard C++ escape sequences per 2.13.2.3 of the C++ standard. _RE_PATTERN_CLEANSE_LINE_ESCAPES = re.compile( r'\\([abfnrtv?"\\\']|\d+|x[0-9a-fA-F]+)') -# Matches strings. Escape codes should already be removed by ESCAPES. -_RE_PATTERN_CLEANSE_LINE_DOUBLE_QUOTES = re.compile(r'"[^"]*"') -# Matches characters. Escape codes should already be removed by ESCAPES. -_RE_PATTERN_CLEANSE_LINE_SINGLE_QUOTES = re.compile(r"'.'") -# Matches multi-line C++ comments. +# Match a single C style comment on the same line. +_RE_PATTERN_C_COMMENTS = r'/\*(?:[^*]|\*(?!/))*\*/' +# Matches multi-line C style comments. # This RE is a little bit more complicated than one might expect, because we # have to take care of space removals tools so we can handle comments inside # statements better. @@ -834,10 +1757,10 @@ def Error(filename, linenum, category, confidence, message): # if this doesn't work we try on left side but only if there's a non-character # on the right. _RE_PATTERN_CLEANSE_LINE_C_COMMENTS = re.compile( - r"""(\s*/\*.*\*/\s*$| - /\*.*\*/\s+| - \s+/\*.*\*/(?=\W)| - /\*.*\*/)""", re.VERBOSE) + r'(\s*' + _RE_PATTERN_C_COMMENTS + r'\s*$|' + + _RE_PATTERN_C_COMMENTS + r'\s+|' + + r'\s+' + _RE_PATTERN_C_COMMENTS + r'(?=\W)|' + + _RE_PATTERN_C_COMMENTS + r')') def IsCppString(line): @@ -857,6 +1780,82 @@ def IsCppString(line): return ((line.count('"') - line.count(r'\"') - line.count("'\"'")) & 1) == 1 +def CleanseRawStrings(raw_lines): + """Removes C++11 raw strings from lines. + + Before: + static const char kData[] = R"( + multi-line string + )"; + + After: + static const char kData[] = "" + (replaced by blank line) + ""; + + Args: + raw_lines: list of raw lines. + + Returns: + list of lines with C++11 raw strings replaced by empty strings. + """ + + delimiter = None + lines_without_raw_strings = [] + for line in raw_lines: + if delimiter: + # Inside a raw string, look for the end + end = line.find(delimiter) + if end >= 0: + # Found the end of the string, match leading space for this + # line and resume copying the original lines, and also insert + # a "" on the last line. + leading_space = Match(r'^(\s*)\S', line) + line = leading_space.group(1) + '""' + line[end + len(delimiter):] + delimiter = None + else: + # Haven't found the end yet, append a blank line. + line = '""' + + # Look for beginning of a raw string, and replace them with + # empty strings. This is done in a loop to handle multiple raw + # strings on the same line. + while delimiter is None: + # Look for beginning of a raw string. + # See 2.14.15 [lex.string] for syntax. + # + # Once we have matched a raw string, we check the prefix of the + # line to make sure that the line is not part of a single line + # comment. It's done this way because we remove raw strings + # before removing comments as opposed to removing comments + # before removing raw strings. This is because there are some + # cpplint checks that requires the comments to be preserved, but + # we don't want to check comments that are inside raw strings. + matched = Match(r'^(.*?)\b(?:R|u8R|uR|UR|LR)"([^\s\\()]*)\((.*)$', line) + if (matched and + not Match(r'^([^\'"]|\'(\\.|[^\'])*\'|"(\\.|[^"])*")*//', + matched.group(1))): + delimiter = ')' + matched.group(2) + '"' + + end = matched.group(3).find(delimiter) + if end >= 0: + # Raw string ended on same line + line = (matched.group(1) + '""' + + matched.group(3)[end + len(delimiter):]) + delimiter = None + else: + # Start of a multi-line raw string + line = matched.group(1) + '""' + else: + break + + lines_without_raw_strings.append(line) + + # TODO(unknown): if delimiter is not None here, we might want to + # emit a warning for unterminated string. + return lines_without_raw_strings + + def FindNextMultiLineCommentStart(lines, lineix): """Find the beginning marker for a multiline comment.""" while lineix < len(lines): @@ -879,10 +1878,10 @@ def FindNextMultiLineCommentEnd(lines, lineix): def RemoveMultiLineCommentsFromRange(lines, begin, end): """Clears a range of lines for multi-line comments.""" - # Having // dummy comments makes the lines non-empty, so we will not get + # Having // comments makes the lines non-empty, so we will not get # unnecessary blank line warnings later in the code. for i in range(begin, end): - lines[i] = '// dummy' + lines[i] = '/**/' def RemoveMultiLineComments(filename, lines, error): @@ -918,12 +1917,14 @@ def CleanseComments(line): class CleansedLines(object): - """Holds 3 copies of all lines with different preprocessing applied to them. - - 1) elided member contains lines without strings and comments, - 2) lines member contains lines without comments, and - 3) raw member contains all the lines without processing. - All these three members are of , and of the same length. + """Holds 4 copies of all lines with different preprocessing applied to them. + + 1) elided member contains lines without strings and comments. + 2) lines member contains lines without comments. + 3) raw_lines member contains all the lines without processing. + 4) lines_without_raw_strings member is same as raw_lines, but with C++11 raw + strings removed. + All these members are of , and of the same length. """ def __init__(self, lines): @@ -931,9 +1932,11 @@ def __init__(self, lines): self.lines = [] self.raw_lines = lines self.num_lines = len(lines) - for linenum in range(len(lines)): - self.lines.append(CleanseComments(lines[linenum])) - elided = self._CollapseStrings(lines[linenum]) + self.lines_without_raw_strings = CleanseRawStrings(lines) + for linenum in range(len(self.lines_without_raw_strings)): + self.lines.append(CleanseComments( + self.lines_without_raw_strings[linenum])) + elided = self._CollapseStrings(self.lines_without_raw_strings[linenum]) self.elided.append(CleanseComments(elided)) def NumLines(self): @@ -952,22 +1955,151 @@ def _CollapseStrings(elided): Returns: The line with collapsed strings. """ - if not _RE_PATTERN_INCLUDE.match(elided): - # Remove escaped characters first to make quote/single quote collapsing - # basic. Things that look like escaped characters shouldn't occur - # outside of strings and chars. - elided = _RE_PATTERN_CLEANSE_LINE_ESCAPES.sub('', elided) - elided = _RE_PATTERN_CLEANSE_LINE_SINGLE_QUOTES.sub("''", elided) - elided = _RE_PATTERN_CLEANSE_LINE_DOUBLE_QUOTES.sub('""', elided) - return elided + if _RE_PATTERN_INCLUDE.match(elided): + return elided + + # Remove escaped characters first to make quote/single quote collapsing + # basic. Things that look like escaped characters shouldn't occur + # outside of strings and chars. + elided = _RE_PATTERN_CLEANSE_LINE_ESCAPES.sub('', elided) + + # Replace quoted strings and digit separators. Both single quotes + # and double quotes are processed in the same loop, otherwise + # nested quotes wouldn't work. + collapsed = '' + while True: + # Find the first quote character + match = Match(r'^([^\'"]*)([\'"])(.*)$', elided) + if not match: + collapsed += elided + break + head, quote, tail = match.groups() + + if quote == '"': + # Collapse double quoted strings + second_quote = tail.find('"') + if second_quote >= 0: + collapsed += head + '""' + elided = tail[second_quote + 1:] + else: + # Unmatched double quote, don't bother processing the rest + # of the line since this is probably a multiline string. + collapsed += elided + break + else: + # Found single quote, check nearby text to eliminate digit separators. + # + # There is no special handling for floating point here, because + # the integer/fractional/exponent parts would all be parsed + # correctly as long as there are digits on both sides of the + # separator. So we are fine as long as we don't see something + # like "0.'3" (gcc 4.9.0 will not allow this literal). + if Search(r'\b(?:0[bBxX]?|[1-9])[0-9a-fA-F]*$', head): + match_literal = Match(r'^((?:\'?[0-9a-zA-Z_])*)(.*)$', "'" + tail) + collapsed += head + match_literal.group(1).replace("'", '') + elided = match_literal.group(2) + else: + second_quote = tail.find('\'') + if second_quote >= 0: + collapsed += head + "''" + elided = tail[second_quote + 1:] + else: + # Unmatched single quote + collapsed += elided + break + + return collapsed + + +def FindEndOfExpressionInLine(line, startpos, stack): + """Find the position just after the end of current parenthesized expression. + + Args: + line: a CleansedLines line. + startpos: start searching at this position. + stack: nesting stack at startpos. + + Returns: + On finding matching end: (index just after matching end, None) + On finding an unclosed expression: (-1, None) + Otherwise: (-1, new stack at end of this line) + """ + for i in xrange(startpos, len(line)): + char = line[i] + if char in '([{': + # Found start of parenthesized expression, push to expression stack + stack.append(char) + elif char == '<': + # Found potential start of template argument list + if i > 0 and line[i - 1] == '<': + # Left shift operator + if stack and stack[-1] == '<': + stack.pop() + if not stack: + return (-1, None) + elif i > 0 and Search(r'\boperator\s*$', line[0:i]): + # operator<, don't add to stack + continue + else: + # Tentative start of template argument list + stack.append('<') + elif char in ')]}': + # Found end of parenthesized expression. + # + # If we are currently expecting a matching '>', the pending '<' + # must have been an operator. Remove them from expression stack. + while stack and stack[-1] == '<': + stack.pop() + if not stack: + return (-1, None) + if ((stack[-1] == '(' and char == ')') or + (stack[-1] == '[' and char == ']') or + (stack[-1] == '{' and char == '}')): + stack.pop() + if not stack: + return (i + 1, None) + else: + # Mismatched parentheses + return (-1, None) + elif char == '>': + # Found potential end of template argument list. + + # Ignore "->" and operator functions + if (i > 0 and + (line[i - 1] == '-' or Search(r'\boperator\s*$', line[0:i - 1]))): + continue + + # Pop the stack if there is a matching '<'. Otherwise, ignore + # this '>' since it must be an operator. + if stack: + if stack[-1] == '<': + stack.pop() + if not stack: + return (i + 1, None) + elif char == ';': + # Found something that look like end of statements. If we are currently + # expecting a '>', the matching '<' must have been an operator, since + # template argument list should not contain statements. + while stack and stack[-1] == '<': + stack.pop() + if not stack: + return (-1, None) + + # Did not find end of expression or unbalanced parentheses on this line + return (-1, stack) def CloseExpression(clean_lines, linenum, pos): - """If input points to ( or { or [, finds the position that closes it. + """If input points to ( or { or [ or <, finds the position that closes it. - If lines[linenum][pos] points to a '(' or '{' or '[', finds the + If lines[linenum][pos] points to a '(' or '{' or '[' or '<', finds the linenum/pos that correspond to the closing of the expression. + TODO(unknown): cpplint spends a fair bit of time matching parentheses. + Ideally we would want to index all opening and closing parentheses once + and have CloseExpression be just a simple lookup, but due to preprocessor + tricks, this is not so easy. + Args: clean_lines: A CleansedLines instance containing the file. linenum: The number of the line to check. @@ -981,31 +2113,146 @@ def CloseExpression(clean_lines, linenum, pos): """ line = clean_lines.elided[linenum] - startchar = line[pos] - if startchar not in '({[': + if (line[pos] not in '({[<') or Match(r'<[<=]', line[pos:]): return (line, clean_lines.NumLines(), -1) - if startchar == '(': endchar = ')' - if startchar == '[': endchar = ']' - if startchar == '{': endchar = '}' - num_open = line.count(startchar) - line.count(endchar) - while linenum < clean_lines.NumLines() and num_open > 0: + # Check first line + (end_pos, stack) = FindEndOfExpressionInLine(line, pos, []) + if end_pos > -1: + return (line, linenum, end_pos) + + # Continue scanning forward + while stack and linenum < clean_lines.NumLines() - 1: linenum += 1 line = clean_lines.elided[linenum] - num_open += line.count(startchar) - line.count(endchar) - # OK, now find the endchar that actually got us back to even - endpos = len(line) - while num_open >= 0: - endpos = line.rfind(')', 0, endpos) - num_open -= 1 # chopped off another ) - return (line, linenum, endpos + 1) + (end_pos, stack) = FindEndOfExpressionInLine(line, 0, stack) + if end_pos > -1: + return (line, linenum, end_pos) + + # Did not find end of expression before end of file, give up + return (line, clean_lines.NumLines(), -1) + + +def FindStartOfExpressionInLine(line, endpos, stack): + """Find position at the matching start of current expression. + + This is almost the reverse of FindEndOfExpressionInLine, but note + that the input position and returned position differs by 1. + + Args: + line: a CleansedLines line. + endpos: start searching at this position. + stack: nesting stack at endpos. + + Returns: + On finding matching start: (index at matching start, None) + On finding an unclosed expression: (-1, None) + Otherwise: (-1, new stack at beginning of this line) + """ + i = endpos + while i >= 0: + char = line[i] + if char in ')]}': + # Found end of expression, push to expression stack + stack.append(char) + elif char == '>': + # Found potential end of template argument list. + # + # Ignore it if it's a "->" or ">=" or "operator>" + if (i > 0 and + (line[i - 1] == '-' or + Match(r'\s>=\s', line[i - 1:]) or + Search(r'\boperator\s*$', line[0:i]))): + i -= 1 + else: + stack.append('>') + elif char == '<': + # Found potential start of template argument list + if i > 0 and line[i - 1] == '<': + # Left shift operator + i -= 1 + else: + # If there is a matching '>', we can pop the expression stack. + # Otherwise, ignore this '<' since it must be an operator. + if stack and stack[-1] == '>': + stack.pop() + if not stack: + return (i, None) + elif char in '([{': + # Found start of expression. + # + # If there are any unmatched '>' on the stack, they must be + # operators. Remove those. + while stack and stack[-1] == '>': + stack.pop() + if not stack: + return (-1, None) + if ((char == '(' and stack[-1] == ')') or + (char == '[' and stack[-1] == ']') or + (char == '{' and stack[-1] == '}')): + stack.pop() + if not stack: + return (i, None) + else: + # Mismatched parentheses + return (-1, None) + elif char == ';': + # Found something that look like end of statements. If we are currently + # expecting a '<', the matching '>' must have been an operator, since + # template argument list should not contain statements. + while stack and stack[-1] == '>': + stack.pop() + if not stack: + return (-1, None) + + i -= 1 + + return (-1, stack) + + +def ReverseCloseExpression(clean_lines, linenum, pos): + """If input points to ) or } or ] or >, finds the position that opens it. + + If lines[linenum][pos] points to a ')' or '}' or ']' or '>', finds the + linenum/pos that correspond to the opening of the expression. + + Args: + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + pos: A position on the line. + + Returns: + A tuple (line, linenum, pos) pointer *at* the opening brace, or + (line, 0, -1) if we never find the matching opening brace. Note + we ignore strings and comments when matching; and the line we + return is the 'cleansed' line at linenum. + """ + line = clean_lines.elided[linenum] + if line[pos] not in ')}]>': + return (line, 0, -1) + + # Check last line + (start_pos, stack) = FindStartOfExpressionInLine(line, pos, []) + if start_pos > -1: + return (line, linenum, start_pos) + + # Continue scanning backward + while stack and linenum > 0: + linenum -= 1 + line = clean_lines.elided[linenum] + (start_pos, stack) = FindStartOfExpressionInLine(line, len(line) - 1, stack) + if start_pos > -1: + return (line, linenum, start_pos) + + # Did not find start of expression before beginning of file, give up + return (line, 0, -1) def CheckForCopyright(filename, lines, error): """Logs an error if no Copyright message appears at the top of the file.""" # We'll say it should occur by line 10. Don't forget there's a - # dummy line at the front. + # placeholder line at the front. for line in xrange(1, min(len(lines), 11)): if re.search(r'Copyright', lines[line], re.I): break else: # means no copyright line was found @@ -1014,6 +2261,46 @@ def CheckForCopyright(filename, lines, error): 'You should have a line: "Copyright [year] "') +def GetIndentLevel(line): + """Return the number of leading spaces in line. + + Args: + line: A string to check. + + Returns: + An integer count of leading spaces, possibly zero. + """ + indent = Match(r'^( *)\S', line) + if indent: + return len(indent.group(1)) + else: + return 0 + +def PathSplitToList(path): + """Returns the path split into a list by the separator. + + Args: + path: An absolute or relative path (e.g. '/a/b/c/' or '../a') + + Returns: + A list of path components (e.g. ['a', 'b', 'c]). + """ + lst = [] + while True: + (head, tail) = os.path.split(path) + if head == path: # absolute paths end + lst.append(head) + break + if tail == path: # relative paths end + lst.append(tail) + break + + path = head + lst.append(tail) + + lst.reverse() + return lst + def GetHeaderGuardCPPVariable(filename): """Returns the CPP variable that should be used as a header guard. @@ -1029,12 +2316,69 @@ def GetHeaderGuardCPPVariable(filename): # Restores original filename in case that cpplint is invoked from Emacs's # flymake. filename = re.sub(r'_flymake\.h$', '.h', filename) + filename = re.sub(r'/\.flymake/([^/]*)$', r'/\1', filename) + # Replace 'c++' with 'cpp'. + filename = filename.replace('C++', 'cpp').replace('c++', 'cpp') fileinfo = FileInfo(filename) - return re.sub(r'[-./\s]', '_', fileinfo.RepositoryName()).upper() + '_' + file_path_from_root = fileinfo.RepositoryName() + + def FixupPathFromRoot(): + if _root_debug: + sys.stderr.write("\n_root fixup, _root = '%s', repository name = '%s'\n" + % (_root, fileinfo.RepositoryName())) + + # Process the file path with the --root flag if it was set. + if not _root: + if _root_debug: + sys.stderr.write("_root unspecified\n") + return file_path_from_root + + def StripListPrefix(lst, prefix): + # f(['x', 'y'], ['w, z']) -> None (not a valid prefix) + if lst[:len(prefix)] != prefix: + return None + # f(['a, 'b', 'c', 'd'], ['a', 'b']) -> ['c', 'd'] + return lst[(len(prefix)):] + + # root behavior: + # --root=subdir , lstrips subdir from the header guard + maybe_path = StripListPrefix(PathSplitToList(file_path_from_root), + PathSplitToList(_root)) + if _root_debug: + sys.stderr.write(("_root lstrip (maybe_path=%s, file_path_from_root=%s," + + " _root=%s)\n") % (maybe_path, file_path_from_root, _root)) -def CheckForHeaderGuard(filename, lines, error): + if maybe_path: + return os.path.join(*maybe_path) + + # --root=.. , will prepend the outer directory to the header guard + full_path = fileinfo.FullName() + # adapt slashes for windows + root_abspath = os.path.abspath(_root).replace('\\', '/') + + maybe_path = StripListPrefix(PathSplitToList(full_path), + PathSplitToList(root_abspath)) + + if _root_debug: + sys.stderr.write(("_root prepend (maybe_path=%s, full_path=%s, " + + "root_abspath=%s)\n") % (maybe_path, full_path, root_abspath)) + + if maybe_path: + return os.path.join(*maybe_path) + + if _root_debug: + sys.stderr.write("_root ignore, returning %s\n" % (file_path_from_root)) + + # --root=FAKE_DIR is ignored + return file_path_from_root + + file_path_from_root = FixupPathFromRoot() + return re.sub(r'[^a-zA-Z0-9]', '_', file_path_from_root).upper() + '_' + + +def CheckForHeaderGuard(filename, clean_lines, error): """Checks that the file contains a header guard. Logs an error if no #ifndef header guard is present. For other @@ -1042,18 +2386,34 @@ def CheckForHeaderGuard(filename, lines, error): Args: filename: The name of the C++ header file. - lines: An array of strings, each representing a line of the file. + clean_lines: A CleansedLines instance containing the file. error: The function to call with any errors found. """ + # Don't check for header guards if there are error suppression + # comments somewhere in this file. + # + # Because this is silencing a warning for a nonexistent line, we + # only support the very specific NOLINT(build/header_guard) syntax, + # and not the general NOLINT or NOLINT(*) syntax. + raw_lines = clean_lines.lines_without_raw_strings + for i in raw_lines: + if Search(r'//\s*NOLINT\(build/header_guard\)', i): + return + + # Allow pragma once instead of header guards + for i in raw_lines: + if Search(r'^\s*#pragma\s+once', i): + return + cppvar = GetHeaderGuardCPPVariable(filename) - ifndef = None + ifndef = '' ifndef_linenum = 0 - define = None - endif = None + define = '' + endif = '' endif_linenum = 0 - for linenum, line in enumerate(lines): + for linenum, line in enumerate(raw_lines): linesplit = line.split() if len(linesplit) >= 2: # find the first occurrence of #ifndef and #define, save arg @@ -1076,34 +2436,95 @@ def CheckForHeaderGuard(filename, lines, error): # The guard should be PATH_FILE_H_, but we also allow PATH_FILE_H__ # for backward compatibility. - #if ifndef != cppvar: - # error_level = 0 - # if ifndef != cppvar + '_': - # error_level = 5 + if ifndef != cppvar: + error_level = 0 + if ifndef != cppvar + '_': + error_level = 5 + + ParseNolintSuppressions(filename, raw_lines[ifndef_linenum], ifndef_linenum, + error) + error(filename, ifndef_linenum, 'build/header_guard', error_level, + '#ifndef header guard has wrong style, please use: %s' % cppvar) + + # Check for "//" comments on endif line. + ParseNolintSuppressions(filename, raw_lines[endif_linenum], endif_linenum, + error) + match = Match(r'#endif\s*//\s*' + cppvar + r'(_)?\b', endif) + if match: + if match.group(1) == '_': + # Issue low severity warning for deprecated double trailing underscore + error(filename, endif_linenum, 'build/header_guard', 0, + '#endif line should be "#endif // %s"' % cppvar) + return + + # Didn't find the corresponding "//" comment. If this file does not + # contain any "//" comments at all, it could be that the compiler + # only wants "/**/" comments, look for those instead. + no_single_line_comments = True + for i in xrange(1, len(raw_lines) - 1): + line = raw_lines[i] + if Match(r'^(?:(?:\'(?:\.|[^\'])*\')|(?:"(?:\.|[^"])*")|[^\'"])*//', line): + no_single_line_comments = False + break + + if no_single_line_comments: + match = Match(r'#endif\s*/\*\s*' + cppvar + r'(_)?\s*\*/', endif) + if match: + if match.group(1) == '_': + # Low severity warning for double trailing underscore + error(filename, endif_linenum, 'build/header_guard', 0, + '#endif line should be "#endif /* %s */"' % cppvar) + return + + # Didn't find anything + error(filename, endif_linenum, 'build/header_guard', 5, + '#endif line should be "#endif // %s"' % cppvar) + + +def CheckHeaderFileIncluded(filename, include_state, error): + """Logs an error if a source file does not include its header.""" + + # Do not check test files + fileinfo = FileInfo(filename) + if Search(_TEST_FILE_SUFFIX, fileinfo.BaseName()): + return + + for ext in GetHeaderExtensions(): + basefilename = filename[0:len(filename) - len(fileinfo.Extension())] + headerfile = basefilename + '.' + ext + if not os.path.exists(headerfile): + continue + headername = FileInfo(headerfile).RepositoryName() + first_include = None + include_uses_unix_dir_aliases = False + for section_list in include_state.include_list: + for f in section_list: + include_text = f[0] + if "./" in include_text: + include_uses_unix_dir_aliases = True + if headername in include_text or include_text in headername: + return + if not first_include: + first_include = f[1] - # ParseNolintSuppressions(filename, lines[ifndef_linenum], ifndef_linenum, - # error) - # error(filename, ifndef_linenum, 'build/header_guard', error_level, - # '#ifndef header guard has wrong style, please use: %s' % cppvar) + message = '%s should include its header file %s' % (fileinfo.RepositoryName(), headername) + if include_uses_unix_dir_aliases: + message += ". Relative paths like . and .. are not allowed." - #if endif != ('#endif // %s' % cppvar): - # error_level = 0 - # if endif != ('#endif // %s' % (cppvar + '_')): - # error_level = 5 + error(filename, first_include, 'build/include', 5, message) - # ParseNolintSuppressions(filename, lines[endif_linenum], endif_linenum, - # error) - # error(filename, endif_linenum, 'build/header_guard', error_level, - # '#endif line should be "#endif // %s"' % cppvar) +def CheckForBadCharacters(filename, lines, error): + """Logs an error for each line containing bad characters. -def CheckForUnicodeReplacementCharacters(filename, lines, error): - """Logs an error for each line containing Unicode replacement characters. + Two kinds of bad characters: - These indicate that either the file contained invalid UTF-8 (likely) - or Unicode replacement characters (which it shouldn't). Note that - it's possible for this to throw off line numbering if the invalid - UTF-8 occurred adjacent to a newline. + 1. Unicode replacement characters: These indicate that either the file + contained invalid UTF-8 (likely) or Unicode replacement characters (which + it shouldn't). Note that it's possible for this to throw off line + numbering if the invalid UTF-8 occurred adjacent to a newline. + + 2. NUL bytes. These are problematic for some tools. Args: filename: The name of the current file. @@ -1111,9 +2532,11 @@ def CheckForUnicodeReplacementCharacters(filename, lines, error): error: The function to call with any errors found. """ for linenum, line in enumerate(lines): - if u'\ufffd' in line: + if unicode_escape_decode('\ufffd') in line: error(filename, linenum, 'readability/utf8', 5, 'Line contains invalid UTF-8 (or Unicode replacement character).') + if '\0' in line: + error(filename, linenum, 'readability/nul', 5, 'Line contains NUL byte.') def CheckForNewlineAtEOF(filename, lines, error): @@ -1165,27 +2588,40 @@ def CheckForMultilineCommentsAndStrings(filename, clean_lines, linenum, error): 'with #if 0...#endif, ' 'or with more clearly structured multi-line comments.') - #if (line.count('"') - line.count('\\"')) % 2: - # error(filename, linenum, 'readability/multiline_string', 5, - # 'Multi-line string ("...") found. This lint script doesn\'t ' - # 'do well with such strings, and may give bogus warnings. They\'re ' - # 'ugly and unnecessary, and you should use concatenation instead".') - - -threading_list = ( - ('asctime(', 'asctime_r('), - ('ctime(', 'ctime_r('), - ('getgrgid(', 'getgrgid_r('), - ('getgrnam(', 'getgrnam_r('), - ('getlogin(', 'getlogin_r('), - ('getpwnam(', 'getpwnam_r('), - ('getpwuid(', 'getpwuid_r('), - ('gmtime(', 'gmtime_r('), - ('localtime(', 'localtime_r('), - ('rand(', 'rand_r('), - ('readdir(', 'readdir_r('), - ('strtok(', 'strtok_r('), - ('ttyname(', 'ttyname_r('), + if (line.count('"') - line.count('\\"')) % 2: + error(filename, linenum, 'readability/multiline_string', 5, + 'Multi-line string ("...") found. This lint script doesn\'t ' + 'do well with such strings, and may give bogus warnings. ' + 'Use C++11 raw strings or concatenation instead.') + + +# (non-threadsafe name, thread-safe alternative, validation pattern) +# +# The validation pattern is used to eliminate false positives such as: +# _rand(); // false positive due to substring match. +# ->rand(); // some member function rand(). +# ACMRandom rand(seed); // some variable named rand. +# ISAACRandom rand(); // another variable named rand. +# +# Basically we require the return value of these functions to be used +# in some expression context on the same line by matching on some +# operator before the function name. This eliminates constructors and +# member function calls. +_UNSAFE_FUNC_PREFIX = r'(?:[-+*/=%^&|(<]\s*|>\s+)' +_THREADING_LIST = ( + ('asctime(', 'asctime_r(', _UNSAFE_FUNC_PREFIX + r'asctime\([^)]+\)'), + ('ctime(', 'ctime_r(', _UNSAFE_FUNC_PREFIX + r'ctime\([^)]+\)'), + ('getgrgid(', 'getgrgid_r(', _UNSAFE_FUNC_PREFIX + r'getgrgid\([^)]+\)'), + ('getgrnam(', 'getgrnam_r(', _UNSAFE_FUNC_PREFIX + r'getgrnam\([^)]+\)'), + ('getlogin(', 'getlogin_r(', _UNSAFE_FUNC_PREFIX + r'getlogin\(\)'), + ('getpwnam(', 'getpwnam_r(', _UNSAFE_FUNC_PREFIX + r'getpwnam\([^)]+\)'), + ('getpwuid(', 'getpwuid_r(', _UNSAFE_FUNC_PREFIX + r'getpwuid\([^)]+\)'), + ('gmtime(', 'gmtime_r(', _UNSAFE_FUNC_PREFIX + r'gmtime\([^)]+\)'), + ('localtime(', 'localtime_r(', _UNSAFE_FUNC_PREFIX + r'localtime\([^)]+\)'), + ('rand(', 'rand_r(', _UNSAFE_FUNC_PREFIX + r'rand\(\)'), + ('strtok(', 'strtok_r(', + _UNSAFE_FUNC_PREFIX + r'strtok\([^)]+\)'), + ('ttyname(', 'ttyname_r(', _UNSAFE_FUNC_PREFIX + r'ttyname\([^)]+\)'), ) @@ -1205,17 +2641,34 @@ def CheckPosixThreading(filename, clean_lines, linenum, error): error: The function to call with any errors found. """ line = clean_lines.elided[linenum] - for single_thread_function, multithread_safe_function in threading_list: - ix = line.find(single_thread_function) - # Comparisons made explicit for clarity -- pylint: disable-msg=C6403 - if ix >= 0 and (ix == 0 or (not line[ix - 1].isalnum() and - line[ix - 1] not in ('_', '.', '>'))): + for single_thread_func, multithread_safe_func, pattern in _THREADING_LIST: + # Additional pattern matching check to confirm that this is the + # function we are looking for + if Search(pattern, line): error(filename, linenum, 'runtime/threadsafe_fn', 2, - 'Consider using ' + multithread_safe_function + - '...) instead of ' + single_thread_function + + 'Consider using ' + multithread_safe_func + + '...) instead of ' + single_thread_func + '...) for improved thread safety.') +def CheckVlogArguments(filename, clean_lines, linenum, error): + """Checks that VLOG() is only used for defining a logging level. + + For example, VLOG(2) is correct. VLOG(INFO), VLOG(WARNING), VLOG(ERROR), and + VLOG(FATAL) are not. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ + line = clean_lines.elided[linenum] + if Search(r'\bVLOG\((INFO|ERROR|WARNING|DFATAL|FATAL)\)', line): + error(filename, linenum, 'runtime/vlog', 5, + 'VLOG() should be used with numeric verbosity level. ' + 'Use LOG() if you want symbolic severity levels.') + # Matches invalid increment: *count++, which moves pointer instead of # incrementing a value. _RE_PATTERN_INVALID_INCREMENT = re.compile( @@ -1244,17 +2697,95 @@ def CheckInvalidIncrement(filename, clean_lines, linenum, error): 'Changing pointer instead of value (or unused value of operator*).') -class _ClassInfo(object): - """Stores information about a class.""" +def IsMacroDefinition(clean_lines, linenum): + if Search(r'^#define', clean_lines[linenum]): + return True + + if linenum > 0 and Search(r'\\$', clean_lines[linenum - 1]): + return True + + return False + + +def IsForwardClassDeclaration(clean_lines, linenum): + return Match(r'^\s*(\btemplate\b)*.*class\s+\w+;\s*$', clean_lines[linenum]) + + +class _BlockInfo(object): + """Stores information about a generic block of code.""" + + def __init__(self, linenum, seen_open_brace): + self.starting_linenum = linenum + self.seen_open_brace = seen_open_brace + self.open_parentheses = 0 + self.inline_asm = _NO_ASM + self.check_namespace_indentation = False + + def CheckBegin(self, filename, clean_lines, linenum, error): + """Run checks that applies to text up to the opening brace. + + This is mostly for checking the text after the class identifier + and the "{", usually where the base class is specified. For other + blocks, there isn't much to check, so we always pass. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ + pass + + def CheckEnd(self, filename, clean_lines, linenum, error): + """Run checks that applies to text after the closing brace. + + This is mostly used for checking end of namespace comments. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ + pass + + def IsBlockInfo(self): + """Returns true if this block is a _BlockInfo. + + This is convenient for verifying that an object is an instance of + a _BlockInfo, but not an instance of any of the derived classes. + + Returns: + True for this class, False for derived classes. + """ + return self.__class__ == _BlockInfo - def __init__(self, name, clean_lines, linenum): + +class _ExternCInfo(_BlockInfo): + """Stores information about an 'extern "C"' block.""" + + def __init__(self, linenum): + _BlockInfo.__init__(self, linenum, True) + + +class _ClassInfo(_BlockInfo): + """Stores information about a class.""" + + def __init__(self, name, class_or_struct, clean_lines, linenum): + _BlockInfo.__init__(self, linenum, False) self.name = name - self.linenum = linenum - self.seen_open_brace = False self.is_derived = False - self.virtual_method_linenumber = None - self.has_virtual_destructor = False - self.brace_depth = 0 + self.check_namespace_indentation = True + if class_or_struct == 'struct': + self.access = 'public' + self.is_struct = True + else: + self.access = 'private' + self.is_struct = False + + # Remember initial indentation level for this class. Using raw_lines here + # instead of elided to account for leading comments. + self.class_indent = GetIndentLevel(clean_lines.raw_lines[linenum]) # Try to find the end of the class. This will be confused by things like: # class A { @@ -1264,45 +2795,500 @@ def __init__(self, name, clean_lines, linenum): self.last_line = 0 depth = 0 for i in range(linenum, clean_lines.NumLines()): - line = clean_lines.lines[i] + line = clean_lines.elided[i] depth += line.count('{') - line.count('}') if not depth: self.last_line = i break + def CheckBegin(self, filename, clean_lines, linenum, error): + # Look for a bare ':' + if Search('(^|[^:]):($|[^:])', clean_lines.elided[linenum]): + self.is_derived = True + + def CheckEnd(self, filename, clean_lines, linenum, error): + # If there is a DISALLOW macro, it should appear near the end of + # the class. + seen_last_thing_in_class = False + for i in xrange(linenum - 1, self.starting_linenum, -1): + match = Search( + r'\b(DISALLOW_COPY_AND_ASSIGN|DISALLOW_IMPLICIT_CONSTRUCTORS)\(' + + self.name + r'\)', + clean_lines.elided[i]) + if match: + if seen_last_thing_in_class: + error(filename, i, 'readability/constructors', 3, + match.group(1) + ' should be the last thing in the class') + break -class _ClassState(object): - """Holds the current state of the parse relating to class declarations. + if not Match(r'^\s*$', clean_lines.elided[i]): + seen_last_thing_in_class = True - It maintains a stack of _ClassInfos representing the parser's guess - as to the current nesting of class declarations. The innermost class - is at the top (back) of the stack. Typically, the stack will either - be empty or have exactly one entry. - """ + # Check that closing brace is aligned with beginning of the class. + # Only do this if the closing brace is indented by only whitespaces. + # This means we will not check single-line class definitions. + indent = Match(r'^( *)\}', clean_lines.elided[linenum]) + if indent and len(indent.group(1)) != self.class_indent: + if self.is_struct: + parent = 'struct ' + self.name + else: + parent = 'class ' + self.name + error(filename, linenum, 'whitespace/indent', 3, + 'Closing brace should be aligned with beginning of %s' % parent) + + +class _NamespaceInfo(_BlockInfo): + """Stores information about a namespace.""" + + def __init__(self, name, linenum): + _BlockInfo.__init__(self, linenum, False) + self.name = name or '' + self.check_namespace_indentation = True + + def CheckEnd(self, filename, clean_lines, linenum, error): + """Check end of namespace comments.""" + line = clean_lines.raw_lines[linenum] + + # Check how many lines is enclosed in this namespace. Don't issue + # warning for missing namespace comments if there aren't enough + # lines. However, do apply checks if there is already an end of + # namespace comment and it's incorrect. + # + # TODO(unknown): We always want to check end of namespace comments + # if a namespace is large, but sometimes we also want to apply the + # check if a short namespace contained nontrivial things (something + # other than forward declarations). There is currently no logic on + # deciding what these nontrivial things are, so this check is + # triggered by namespace size only, which works most of the time. + if (linenum - self.starting_linenum < 10 + and not Match(r'^\s*};*\s*(//|/\*).*\bnamespace\b', line)): + return + + # Look for matching comment at end of namespace. + # + # Note that we accept C style "/* */" comments for terminating + # namespaces, so that code that terminate namespaces inside + # preprocessor macros can be cpplint clean. + # + # We also accept stuff like "// end of namespace ." with the + # period at the end. + # + # Besides these, we don't accept anything else, otherwise we might + # get false negatives when existing comment is a substring of the + # expected namespace. + if self.name: + # Named namespace + if not Match((r'^\s*};*\s*(//|/\*).*\bnamespace\s+' + + re.escape(self.name) + r'[\*/\.\\\s]*$'), + line): + error(filename, linenum, 'readability/namespace', 5, + 'Namespace should be terminated with "// namespace %s"' % + self.name) + else: + # Anonymous namespace + if not Match(r'^\s*};*\s*(//|/\*).*\bnamespace[\*/\.\\\s]*$', line): + # If "// namespace anonymous" or "// anonymous namespace (more text)", + # mention "// anonymous namespace" as an acceptable form + if Match(r'^\s*}.*\b(namespace anonymous|anonymous namespace)\b', line): + error(filename, linenum, 'readability/namespace', 5, + 'Anonymous namespace should be terminated with "// namespace"' + ' or "// anonymous namespace"') + else: + error(filename, linenum, 'readability/namespace', 5, + 'Anonymous namespace should be terminated with "// namespace"') + + +class _PreprocessorInfo(object): + """Stores checkpoints of nesting stacks when #if/#else is seen.""" + + def __init__(self, stack_before_if): + # The entire nesting stack before #if + self.stack_before_if = stack_before_if + + # The entire nesting stack up to #else + self.stack_before_else = [] + + # Whether we have already seen #else or #elif + self.seen_else = False + + +class NestingState(object): + """Holds states related to parsing braces.""" def __init__(self): - self.classinfo_stack = [] + # Stack for tracking all braces. An object is pushed whenever we + # see a "{", and popped when we see a "}". Only 3 types of + # objects are possible: + # - _ClassInfo: a class or struct. + # - _NamespaceInfo: a namespace. + # - _BlockInfo: some other type of block. + self.stack = [] + + # Top of the previous stack before each Update(). + # + # Because the nesting_stack is updated at the end of each line, we + # had to do some convoluted checks to find out what is the current + # scope at the beginning of the line. This check is simplified by + # saving the previous top of nesting stack. + # + # We could save the full stack, but we only need the top. Copying + # the full nesting stack would slow down cpplint by ~10%. + self.previous_stack_top = [] + + # Stack of _PreprocessorInfo objects. + self.pp_stack = [] + + def SeenOpenBrace(self): + """Check if we have seen the opening brace for the innermost block. + + Returns: + True if we have seen the opening brace, False if the innermost + block is still expecting an opening brace. + """ + return (not self.stack) or self.stack[-1].seen_open_brace + + def InNamespaceBody(self): + """Check if we are currently one level inside a namespace body. + + Returns: + True if top of the stack is a namespace block, False otherwise. + """ + return self.stack and isinstance(self.stack[-1], _NamespaceInfo) + + def InExternC(self): + """Check if we are currently one level inside an 'extern "C"' block. + + Returns: + True if top of the stack is an extern block, False otherwise. + """ + return self.stack and isinstance(self.stack[-1], _ExternCInfo) + + def InClassDeclaration(self): + """Check if we are currently one level inside a class or struct declaration. + + Returns: + True if top of the stack is a class/struct, False otherwise. + """ + return self.stack and isinstance(self.stack[-1], _ClassInfo) + + def InAsmBlock(self): + """Check if we are currently one level inside an inline ASM block. + + Returns: + True if the top of the stack is a block containing inline ASM. + """ + return self.stack and self.stack[-1].inline_asm != _NO_ASM + + def InTemplateArgumentList(self, clean_lines, linenum, pos): + """Check if current position is inside template argument list. + + Args: + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + pos: position just after the suspected template argument. + Returns: + True if (linenum, pos) is inside template arguments. + """ + while linenum < clean_lines.NumLines(): + # Find the earliest character that might indicate a template argument + line = clean_lines.elided[linenum] + match = Match(r'^[^{};=\[\]\.<>]*(.)', line[pos:]) + if not match: + linenum += 1 + pos = 0 + continue + token = match.group(1) + pos += len(match.group(0)) + + # These things do not look like template argument list: + # class Suspect { + # class Suspect x; } + if token in ('{', '}', ';'): return False + + # These things look like template argument list: + # template + # template + # template + # template + if token in ('>', '=', '[', ']', '.'): return True + + # Check if token is an unmatched '<'. + # If not, move on to the next character. + if token != '<': + pos += 1 + if pos >= len(line): + linenum += 1 + pos = 0 + continue + + # We can't be sure if we just find a single '<', and need to + # find the matching '>'. + (_, end_line, end_pos) = CloseExpression(clean_lines, linenum, pos - 1) + if end_pos < 0: + # Not sure if template argument list or syntax error in file + return False + linenum = end_line + pos = end_pos + return False + + def UpdatePreprocessor(self, line): + """Update preprocessor stack. + + We need to handle preprocessors due to classes like this: + #ifdef SWIG + struct ResultDetailsPageElementExtensionPoint { + #else + struct ResultDetailsPageElementExtensionPoint : public Extension { + #endif + + We make the following assumptions (good enough for most files): + - Preprocessor condition evaluates to true from #if up to first + #else/#elif/#endif. + + - Preprocessor condition evaluates to false from #else/#elif up + to #endif. We still perform lint checks on these lines, but + these do not affect nesting stack. + + Args: + line: current line to check. + """ + if Match(r'^\s*#\s*(if|ifdef|ifndef)\b', line): + # Beginning of #if block, save the nesting stack here. The saved + # stack will allow us to restore the parsing state in the #else case. + self.pp_stack.append(_PreprocessorInfo(copy.deepcopy(self.stack))) + elif Match(r'^\s*#\s*(else|elif)\b', line): + # Beginning of #else block + if self.pp_stack: + if not self.pp_stack[-1].seen_else: + # This is the first #else or #elif block. Remember the + # whole nesting stack up to this point. This is what we + # keep after the #endif. + self.pp_stack[-1].seen_else = True + self.pp_stack[-1].stack_before_else = copy.deepcopy(self.stack) + + # Restore the stack to how it was before the #if + self.stack = copy.deepcopy(self.pp_stack[-1].stack_before_if) + else: + # TODO(unknown): unexpected #else, issue warning? + pass + elif Match(r'^\s*#\s*endif\b', line): + # End of #if or #else blocks. + if self.pp_stack: + # If we saw an #else, we will need to restore the nesting + # stack to its former state before the #else, otherwise we + # will just continue from where we left off. + if self.pp_stack[-1].seen_else: + # Here we can just use a shallow copy since we are the last + # reference to it. + self.stack = self.pp_stack[-1].stack_before_else + # Drop the corresponding #if + self.pp_stack.pop() + else: + # TODO(unknown): unexpected #endif, issue warning? + pass + + # TODO(unknown): Update() is too long, but we will refactor later. + def Update(self, filename, clean_lines, linenum, error): + """Update nesting state with current line. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ + line = clean_lines.elided[linenum] + + # Remember top of the previous nesting stack. + # + # The stack is always pushed/popped and not modified in place, so + # we can just do a shallow copy instead of copy.deepcopy. Using + # deepcopy would slow down cpplint by ~28%. + if self.stack: + self.previous_stack_top = self.stack[-1] + else: + self.previous_stack_top = None + + # Update pp_stack + self.UpdatePreprocessor(line) + + # Count parentheses. This is to avoid adding struct arguments to + # the nesting stack. + if self.stack: + inner_block = self.stack[-1] + depth_change = line.count('(') - line.count(')') + inner_block.open_parentheses += depth_change + + # Also check if we are starting or ending an inline assembly block. + if inner_block.inline_asm in (_NO_ASM, _END_ASM): + if (depth_change != 0 and + inner_block.open_parentheses == 1 and + _MATCH_ASM.match(line)): + # Enter assembly block + inner_block.inline_asm = _INSIDE_ASM + else: + # Not entering assembly block. If previous line was _END_ASM, + # we will now shift to _NO_ASM state. + inner_block.inline_asm = _NO_ASM + elif (inner_block.inline_asm == _INSIDE_ASM and + inner_block.open_parentheses == 0): + # Exit assembly block + inner_block.inline_asm = _END_ASM + + # Consume namespace declaration at the beginning of the line. Do + # this in a loop so that we catch same line declarations like this: + # namespace proto2 { namespace bridge { class MessageSet; } } + while True: + # Match start of namespace. The "\b\s*" below catches namespace + # declarations even if it weren't followed by a whitespace, this + # is so that we don't confuse our namespace checker. The + # missing spaces will be flagged by CheckSpacing. + namespace_decl_match = Match(r'^\s*namespace\b\s*([:\w]+)?(.*)$', line) + if not namespace_decl_match: + break + + new_namespace = _NamespaceInfo(namespace_decl_match.group(1), linenum) + self.stack.append(new_namespace) + + line = namespace_decl_match.group(2) + if line.find('{') != -1: + new_namespace.seen_open_brace = True + line = line[line.find('{') + 1:] + + # Look for a class declaration in whatever is left of the line + # after parsing namespaces. The regexp accounts for decorated classes + # such as in: + # class LOCKABLE API Object { + # }; + class_decl_match = Match( + r'^(\s*(?:template\s*<[\w\s<>,:=]*>\s*)?' + r'(class|struct)\s+(?:[a-zA-Z0-9_]+\s+)*(\w+(?:::\w+)*))' + r'(.*)$', line) + if (class_decl_match and + (not self.stack or self.stack[-1].open_parentheses == 0)): + # We do not want to accept classes that are actually template arguments: + # template , + # template class Ignore3> + # void Function() {}; + # + # To avoid template argument cases, we scan forward and look for + # an unmatched '>'. If we see one, assume we are inside a + # template argument list. + end_declaration = len(class_decl_match.group(1)) + if not self.InTemplateArgumentList(clean_lines, linenum, end_declaration): + self.stack.append(_ClassInfo( + class_decl_match.group(3), class_decl_match.group(2), + clean_lines, linenum)) + line = class_decl_match.group(4) + + # If we have not yet seen the opening brace for the innermost block, + # run checks here. + if not self.SeenOpenBrace(): + self.stack[-1].CheckBegin(filename, clean_lines, linenum, error) + + # Update access control if we are inside a class/struct + if self.stack and isinstance(self.stack[-1], _ClassInfo): + classinfo = self.stack[-1] + access_match = Match( + r'^(.*)\b(public|private|protected|signals)(\s+(?:slots\s*)?)?' + r':(?:[^:]|$)', + line) + if access_match: + classinfo.access = access_match.group(2) + + # Check that access keywords are indented +1 space. Skip this + # check if the keywords are not preceded by whitespaces. + indent = access_match.group(1) + if (len(indent) != classinfo.class_indent + 1 and + Match(r'^\s*$', indent)): + if classinfo.is_struct: + parent = 'struct ' + classinfo.name + else: + parent = 'class ' + classinfo.name + slots = '' + if access_match.group(3): + slots = access_match.group(3) + error(filename, linenum, 'whitespace/indent', 3, + '%s%s: should be indented +1 space inside %s' % ( + access_match.group(2), slots, parent)) + + # Consume braces or semicolons from what's left of the line + while True: + # Match first brace, semicolon, or closed parenthesis. + matched = Match(r'^[^{;)}]*([{;)}])(.*)$', line) + if not matched: + break + + token = matched.group(1) + if token == '{': + # If namespace or class hasn't seen a opening brace yet, mark + # namespace/class head as complete. Push a new block onto the + # stack otherwise. + if not self.SeenOpenBrace(): + self.stack[-1].seen_open_brace = True + elif Match(r'^extern\s*"[^"]*"\s*\{', line): + self.stack.append(_ExternCInfo(linenum)) + else: + self.stack.append(_BlockInfo(linenum, True)) + if _MATCH_ASM.match(line): + self.stack[-1].inline_asm = _BLOCK_ASM + + elif token == ';' or token == ')': + # If we haven't seen an opening brace yet, but we already saw + # a semicolon, this is probably a forward declaration. Pop + # the stack for these. + # + # Similarly, if we haven't seen an opening brace yet, but we + # already saw a closing parenthesis, then these are probably + # function arguments with extra "class" or "struct" keywords. + # Also pop these stack for these. + if not self.SeenOpenBrace(): + self.stack.pop() + else: # token == '}' + # Perform end of block checks and pop the stack. + if self.stack: + self.stack[-1].CheckEnd(filename, clean_lines, linenum, error) + self.stack.pop() + line = matched.group(2) + + def InnermostClass(self): + """Get class info on the top of the stack. + + Returns: + A _ClassInfo object if we are inside a class, or None otherwise. + """ + for i in range(len(self.stack), 0, -1): + classinfo = self.stack[i - 1] + if isinstance(classinfo, _ClassInfo): + return classinfo + return None - def CheckFinished(self, filename, error): - """Checks that all classes have been completely parsed. + def CheckCompletedBlocks(self, filename, error): + """Checks that all classes and namespaces have been completely parsed. Call this when all lines in a file have been processed. Args: filename: The name of the current file. error: The function to call with any errors found. """ - if self.classinfo_stack: - # Note: This test can result in false positives if #ifdef constructs - # get in the way of brace matching. See the testBuildClass test in - # cpplint_unittest.py for an example of this. - error(filename, self.classinfo_stack[0].linenum, 'build/class', 5, - 'Failed to find complete declaration of class %s' % - self.classinfo_stack[0].name) + # Note: This test can result in false positives if #ifdef constructs + # get in the way of brace matching. See the testBuildClass test in + # cpplint_unittest.py for an example of this. + for obj in self.stack: + if isinstance(obj, _ClassInfo): + error(filename, obj.starting_linenum, 'build/class', 5, + 'Failed to find complete declaration of class %s' % + obj.name) + elif isinstance(obj, _NamespaceInfo): + error(filename, obj.starting_linenum, 'build/namespaces', 5, + 'Failed to find complete declaration of namespace %s' % + obj.name) def CheckForNonStandardConstructs(filename, clean_lines, linenum, - class_state, error): - """Logs an error if we see certain non-ANSI constructs ignored by gcc-2. + nesting_state, error): + r"""Logs an error if we see certain non-ANSI constructs ignored by gcc-2. Complain about several constructs which gcc-2 accepts, but which are not standard C++. Warning about these in lint is one way to ease the @@ -1314,8 +3300,6 @@ def CheckForNonStandardConstructs(filename, clean_lines, linenum, - text after #endif is not allowed. - invalid inner-style forward declaration. - >? and ?= and ,:]*>\s*)?' - '(class|struct)\s+([A-Z_]+\s+)*(\w+(::\w+)*)', line) - if class_decl_match: - classinfo_stack.append(_ClassInfo( - class_decl_match.group(4), clean_lines, linenum)) - - # Everything else in this function uses the top of the stack if it's - # not empty. - if not classinfo_stack: + # Everything else in this function operates on class declarations. + # Return early if the top of the nesting stack is not a class, or if + # the class head is not completed yet. + classinfo = nesting_state.InnermostClass() + if not classinfo or not classinfo.seen_open_brace: return - classinfo = classinfo_stack[-1] - - # If the opening brace hasn't been seen look for it and also - # parent class declarations. - if not classinfo.seen_open_brace: - # If the line has a ';' in it, assume it's a forward declaration or - # a single-line class declaration, which we won't process. - if line.find(';') != -1: - classinfo_stack.pop() - return - classinfo.seen_open_brace = (line.find('{') != -1) - # Look for a bare ':' - if Search('(^|[^:]):($|[^:])', line): - classinfo.is_derived = True - if not classinfo.seen_open_brace: - return # Everything else in this function is for after open brace - # The class may have been declared with namespace or classname qualifiers. # The constructor and destructor will not have those qualifiers. base_classname = classinfo.name.split('::')[-1] # Look for single-argument constructors that aren't marked explicit. # Technically a valid construct, but against style. - args = Match(r'\s+(?:inline\s+)?%s\s*\(([^,()]+)\)' - % re.escape(base_classname), - line) - if (args and - args.group(1) != 'void' and - not Match(r'(const\s+)?%s\s*(?:<\w+>\s*)?&' % re.escape(base_classname), - args.group(1).strip())): - error(filename, linenum, 'runtime/explicit', 5, - 'Single-argument constructors should be marked explicit.') - - # Look for methods declared virtual. - if Search(r'\bvirtual\b', line): - classinfo.virtual_method_linenumber = linenum - # Only look for a destructor declaration on the same line. It would - # be extremely unlikely for the destructor declaration to occupy - # more than one line. - if Search(r'~%s\s*\(' % base_classname, line): - classinfo.has_virtual_destructor = True - - # Look for class end. - brace_depth = classinfo.brace_depth - brace_depth = brace_depth + line.count('{') - line.count('}') - if brace_depth <= 0: - classinfo = classinfo_stack.pop() - # Try to detect missing virtual destructor declarations. - # For now, only warn if a non-derived class with virtual methods lacks - # a virtual destructor. This is to make it less likely that people will - # declare derived virtual destructors without declaring the base - # destructor virtual. - if ((classinfo.virtual_method_linenumber is not None) and - (not classinfo.has_virtual_destructor) and - (not classinfo.is_derived)): # Only warn for base classes - error(filename, classinfo.linenum, 'runtime/virtual', 4, - 'The class %s probably needs a virtual destructor due to ' - 'having virtual method(s), one declared at line %d.' - % (classinfo.name, classinfo.virtual_method_linenumber)) - else: - classinfo.brace_depth = brace_depth + explicit_constructor_match = Match( + r'\s+(?:(?:inline|constexpr)\s+)*(explicit\s+)?' + r'(?:(?:inline|constexpr)\s+)*%s\s*' + r'\(((?:[^()]|\([^()]*\))*)\)' + % re.escape(base_classname), + line) + + if explicit_constructor_match: + is_marked_explicit = explicit_constructor_match.group(1) + if not explicit_constructor_match.group(2): + constructor_args = [] + else: + constructor_args = explicit_constructor_match.group(2).split(',') + + # collapse arguments so that commas in template parameter lists and function + # argument parameter lists don't split arguments in two + i = 0 + while i < len(constructor_args): + constructor_arg = constructor_args[i] + while (constructor_arg.count('<') > constructor_arg.count('>') or + constructor_arg.count('(') > constructor_arg.count(')')): + constructor_arg += ',' + constructor_args[i + 1] + del constructor_args[i + 1] + constructor_args[i] = constructor_arg + i += 1 + + variadic_args = [arg for arg in constructor_args if '&&...' in arg] + defaulted_args = [arg for arg in constructor_args if '=' in arg] + noarg_constructor = (not constructor_args or # empty arg list + # 'void' arg specifier + (len(constructor_args) == 1 and + constructor_args[0].strip() == 'void')) + onearg_constructor = ((len(constructor_args) == 1 and # exactly one arg + not noarg_constructor) or + # all but at most one arg defaulted + (len(constructor_args) >= 1 and + not noarg_constructor and + len(defaulted_args) >= len(constructor_args) - 1) or + # variadic arguments with zero or one argument + (len(constructor_args) <= 2 and + len(variadic_args) >= 1)) + initializer_list_constructor = bool( + onearg_constructor and + Search(r'\bstd\s*::\s*initializer_list\b', constructor_args[0])) + copy_constructor = bool( + onearg_constructor and + Match(r'((const\s+(volatile\s+)?)?|(volatile\s+(const\s+)?))?' + r'%s(\s*<[^>]*>)?(\s+const)?\s*(?:<\w+>\s*)?&' + % re.escape(base_classname), constructor_args[0].strip())) + + if (not is_marked_explicit and + onearg_constructor and + not initializer_list_constructor and + not copy_constructor): + if defaulted_args or variadic_args: + error(filename, linenum, 'runtime/explicit', 5, + 'Constructors callable with one argument ' + 'should be marked explicit.') + else: + error(filename, linenum, 'runtime/explicit', 5, + 'Single-parameter constructors should be marked explicit.') + elif is_marked_explicit and not onearg_constructor: + if noarg_constructor: + error(filename, linenum, 'runtime/explicit', 5, + 'Zero-parameter constructors should not be marked explicit.') -def CheckSpacingForFunctionCall(filename, line, linenum, error): + +def CheckSpacingForFunctionCall(filename, clean_lines, linenum, error): """Checks for the correctness of various spacing around function calls. Args: filename: The name of the current file. - line: The text of the line to check. + clean_lines: A CleansedLines instance containing the file. linenum: The number of the line to check. error: The function to call with any errors found. """ + line = clean_lines.elided[linenum] # Since function calls often occur inside if/for/while/switch # expressions - which have their own, more liberal conventions - we @@ -1508,7 +3490,8 @@ def CheckSpacingForFunctionCall(filename, line, linenum, error): # Note that we assume the contents of [] to be short enough that # they'll never need to wrap. if ( # Ignore control structures. - not Search(r'\b(if|for|while|switch|return|delete)\b', fncall) and + not Search(r'\b(if|for|while|switch|return|new|delete|catch|sizeof)\b', + fncall) and # Ignore pointers/references to functions. not Search(r' \([^)]+\)\([^)]*(\)|,$)', fncall) and # Ignore pointers/references to arrays. @@ -1520,10 +3503,18 @@ def CheckSpacingForFunctionCall(filename, line, linenum, error): error(filename, linenum, 'whitespace/parens', 2, 'Extra space after (') if (Search(r'\w\s+\(', fncall) and - not Search(r'#\s*define|typedef', fncall) and - not Search(r'<.*',fncall)): - error(filename, linenum, 'whitespace/parens', 4, - 'Extra space before ( in function call') + not Search(r'_{0,2}asm_{0,2}\s+_{0,2}volatile_{0,2}\s+\(', fncall) and + not Search(r'#\s*define|typedef|using\s+\w+\s*=', fncall) and + not Search(r'\w\s+\((\w+::)*\*\w+\)\(', fncall) and + not Search(r'\bcase\s+\(', fncall)): + # TODO(unknown): Space after an operator function seem to be a common + # error, silence those for now by restricting them to highest verbosity. + if Search(r'\boperator_*\b', line): + error(filename, linenum, 'whitespace/parens', 0, + 'Extra space before ( in function call') + else: + error(filename, linenum, 'whitespace/parens', 4, + 'Extra space before ( in function call') # If the ) is followed only by a newline or a { + newline, assume it's # part of a control statement (if/while/etc), and don't complain if Search(r'[^)]\s+\)\s*[^{\s]', fncall): @@ -1552,12 +3543,26 @@ def IsBlankLine(line): return not line or line.isspace() +def CheckForNamespaceIndentation(filename, nesting_state, clean_lines, line, + error): + is_namespace_indent_item = ( + len(nesting_state.stack) > 1 and + nesting_state.stack[-1].check_namespace_indentation and + isinstance(nesting_state.previous_stack_top, _NamespaceInfo) and + nesting_state.previous_stack_top == nesting_state.stack[-2]) + + if ShouldCheckNamespaceIndentation(nesting_state, is_namespace_indent_item, + clean_lines.elided, line): + CheckItemIndentationInNamespace(filename, clean_lines.elided, + line, error) + + def CheckForFunctionLengths(filename, clean_lines, linenum, function_state, error): """Reports for long function bodies. For an overview why this is done, see: - http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Write_Short_Functions + https://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Write_Short_Functions Uses a simplistic algorithm assuming other style guidelines (especially spacing) are followed. @@ -1577,8 +3582,6 @@ def CheckForFunctionLengths(filename, clean_lines, linenum, """ lines = clean_lines.lines line = lines[linenum] - raw = clean_lines.raw_lines - raw_line = raw[linenum] joined_line = '' starting_func = False @@ -1600,7 +3603,7 @@ def CheckForFunctionLengths(filename, clean_lines, linenum, if Search(r'(;|})', start_line): # Declarations and trivial functions body_found = True break # ... ignore - elif Search(r'{', start_line): + if Search(r'{', start_line): body_found = True function = Search(r'((\w|:)*)\(', line).group(1) if Match(r'TEST', function): # Handle TEST... macros @@ -1625,37 +3628,61 @@ def CheckForFunctionLengths(filename, clean_lines, linenum, _RE_PATTERN_TODO = re.compile(r'^//(\s*)TODO(\(.+?\))?:?(\s|$)?') -def CheckComment(comment, filename, linenum, error): - """Checks for common mistakes in TODO comments. +def CheckComment(line, filename, linenum, next_line_start, error): + """Checks for common mistakes in comments. Args: - comment: The text of the comment from the line in question. + line: The line in question. filename: The name of the current file. linenum: The number of the line to check. + next_line_start: The first non-whitespace column of the next line. error: The function to call with any errors found. """ - match = _RE_PATTERN_TODO.match(comment) - if match: - # One whitespace is correct; zero whitespace is handled elsewhere. - leading_whitespace = match.group(1) - if len(leading_whitespace) > 1: - error(filename, linenum, 'whitespace/todo', 2, - 'Too many spaces before TODO') - - #username = match.group(2) - #if not username: - # error(filename, linenum, 'readability/todo', 2, - # 'Missing username in TODO; it should look like ' - # '"// TODO(my_username): Stuff."') - - middle_whitespace = match.group(3) - # Comparisons made explicit for correctness -- pylint: disable-msg=C6403 - if middle_whitespace != ' ' and middle_whitespace != '': - error(filename, linenum, 'whitespace/todo', 2, - 'TODO(my_username) should be followed by a space') - - -def CheckSpacing(filename, clean_lines, linenum, error): + commentpos = line.find('//') + if commentpos != -1: + # Check if the // may be in quotes. If so, ignore it + if re.sub(r'\\.', '', line[0:commentpos]).count('"') % 2 == 0: + # Allow one space for new scopes, two spaces otherwise: + if (not (Match(r'^.*{ *//', line) and next_line_start == commentpos) and + ((commentpos >= 1 and + line[commentpos-1] not in string.whitespace) or + (commentpos >= 2 and + line[commentpos-2] not in string.whitespace))): + error(filename, linenum, 'whitespace/comments', 2, + 'At least two spaces is best between code and comments') + + # Checks for common mistakes in TODO comments. + comment = line[commentpos:] + match = _RE_PATTERN_TODO.match(comment) + if match: + # One whitespace is correct; zero whitespace is handled elsewhere. + leading_whitespace = match.group(1) + if len(leading_whitespace) > 1: + error(filename, linenum, 'whitespace/todo', 2, + 'Too many spaces before TODO') + + username = match.group(2) + if not username: + error(filename, linenum, 'readability/todo', 2, + 'Missing username in TODO; it should look like ' + '"// TODO(my_username): Stuff."') + + middle_whitespace = match.group(3) + # Comparisons made explicit for correctness -- pylint: disable=g-explicit-bool-comparison + if middle_whitespace != ' ' and middle_whitespace != '': + error(filename, linenum, 'whitespace/todo', 2, + 'TODO(my_username) should be followed by a space') + + # If the comment contains an alphanumeric character, there + # should be a space somewhere between it and the // unless + # it's a /// or //! Doxygen comment. + if (Match(r'//[^ ]*\w', comment) and + not Match(r'(///|//\!)(\s+|$)', comment)): + error(filename, linenum, 'whitespace/comments', 4, + 'Should have a space between // and comment') + + +def CheckSpacing(filename, clean_lines, linenum, nesting_state, error): """Checks for the correctness of various spacing issues in the code. Things we check for: spaces around operators, spaces after @@ -1668,16 +3695,35 @@ def CheckSpacing(filename, clean_lines, linenum, error): filename: The name of the current file. clean_lines: A CleansedLines instance containing the file. linenum: The number of the line to check. + nesting_state: A NestingState instance which maintains information about + the current stack of nested blocks being parsed. error: The function to call with any errors found. """ - raw = clean_lines.raw_lines + # Don't use "elided" lines here, otherwise we can't check commented lines. + # Don't want to use "raw" either, because we don't want to check inside C++11 + # raw strings, + raw = clean_lines.lines_without_raw_strings line = raw[linenum] # Before nixing comments, check if the line is blank for no good # reason. This includes the first line after a block is opened, and # blank lines at the end of a function (ie, right before a line like '}' - if IsBlankLine(line): + # + # Skip all the blank line checks if we are immediately inside a + # namespace body. In other words, don't issue blank line warnings + # for this block: + # namespace { + # + # } + # + # A warning about missing end of namespace comments will be issued instead. + # + # Also skip blank line checks for 'extern "C"' blocks, which are formatted + # like namespaces. + if (IsBlankLine(line) and + not nesting_state.InNamespaceBody() and + not nesting_state.InExternC()): elided = clean_lines.elided prev_line = elided[linenum - 1] prevbrace = prev_line.rfind('{') @@ -1685,8 +3731,7 @@ def CheckSpacing(filename, clean_lines, linenum, error): # both start with alnums and are indented the same amount. # This ignores whitespace at the start of a namespace block # because those are not usually indented. - if (prevbrace != -1 and prev_line[prevbrace:].find('}') == -1 - and prev_line[:prevbrace].find('namespace') == -1): + if prevbrace != -1 and prev_line[prevbrace:].find('}') == -1: # OK, we have a blank line at the start of a code block. Before we # complain, we check if it is an exception to the rule: The previous # non-empty line has the parameters of a function header that are indented @@ -1717,13 +3762,9 @@ def CheckSpacing(filename, clean_lines, linenum, error): if not exception: error(filename, linenum, 'whitespace/blank_line', 2, - 'Blank line at the start of a code block. Is this needed?') - # This doesn't ignore whitespace at the end of a namespace block - # because that is too hard without pairing open/close braces; - # however, a special exception is made for namespace closing - # brackets which have a comment containing "namespace". - # - # Also, ignore blank lines at the end of a block in a long if-else + 'Redundant blank line at the start of a code block ' + 'should be deleted.') + # Ignore blank lines at the end of a block in a long if-else # chain, like this: # if (condition1) { # // Something followed by a blank line @@ -1735,59 +3776,75 @@ def CheckSpacing(filename, clean_lines, linenum, error): next_line = raw[linenum + 1] if (next_line and Match(r'\s*}', next_line) - and next_line.find('namespace') == -1 and next_line.find('} else ') == -1): error(filename, linenum, 'whitespace/blank_line', 3, - 'Blank line at the end of a code block. Is this needed?') + 'Redundant blank line at the end of a code block ' + 'should be deleted.') - #matched = Match(r'\s*(public|protected|private):', prev_line) - #if matched: - # error(filename, linenum, 'whitespace/blank_line', 3, - # 'Do not leave a blank line after "%s:"' % matched.group(1)) + matched = Match(r'\s*(public|protected|private):', prev_line) + if matched: + error(filename, linenum, 'whitespace/blank_line', 3, + 'Do not leave a blank line after "%s:"' % matched.group(1)) - # Next, we complain if there's a comment too near the text - commentpos = line.find('//') - if commentpos != -1: - # Check if the // may be in quotes. If so, ignore it - # Comparisons made explicit for clarity -- pylint: disable-msg=C6403 - if (line.count('"', 0, commentpos) - - line.count('\\"', 0, commentpos)) % 2 == 0: # not in quotes - # Allow one space for new scopes, two spaces otherwise: - if (not Match(r'^\s*{ //', line) and - ((commentpos >= 1 and - line[commentpos-1] not in string.whitespace) or - (commentpos >= 2 and - line[commentpos-2] not in string.whitespace))): - error(filename, linenum, 'whitespace/comments', 2, - 'At least two spaces is best between code and comments') - # There should always be a space between the // and the comment - commentend = commentpos + 2 - if commentend < len(line) and not line[commentend] == ' ': - # but some lines are exceptions -- e.g. if they're big - # comment delimiters like: - # //---------------------------------------------------------- - # or are an empty C++ style Doxygen comment, like: - # /// - # or they begin with multiple slashes followed by a space: - # //////// Header comment - match = (Search(r'[=/-]{4,}\s*$', line[commentend:]) or - Search(r'^/$', line[commentend:]) or - Search(r'^/+ ', line[commentend:])) - if not match: - error(filename, linenum, 'whitespace/comments', 4, - 'Should have a space between // and comment') - CheckComment(line[commentpos:], filename, linenum, error) - - line = clean_lines.elided[linenum] # get rid of comments and strings - - # Don't try to do spacing checks for operator methods - line = re.sub(r'operator(==|!=|<|<<|<=|>=|>>|>)\(', 'operator\(', line) + # Next, check comments + next_line_start = 0 + if linenum + 1 < clean_lines.NumLines(): + next_line = raw[linenum + 1] + next_line_start = len(next_line) - len(next_line.lstrip()) + CheckComment(line, filename, linenum, next_line_start, error) + + # get rid of comments and strings + line = clean_lines.elided[linenum] + + # You shouldn't have spaces before your brackets, except for C++11 attributes + # or maybe after 'delete []', 'return []() {};', or 'auto [abc, ...] = ...;'. + if (Search(r'\w\s+\[(?!\[)', line) and + not Search(r'(?:auto&?|delete|return)\s+\[', line)): + error(filename, linenum, 'whitespace/braces', 5, + 'Extra space before [') + + # In range-based for, we wanted spaces before and after the colon, but + # not around "::" tokens that might appear. + if (Search(r'for *\(.*[^:]:[^: ]', line) or + Search(r'for *\(.*[^: ]:[^:]', line)): + error(filename, linenum, 'whitespace/forcolon', 2, + 'Missing space around colon in range-based for loop') + + +def CheckOperatorSpacing(filename, clean_lines, linenum, error): + """Checks for horizontal spacing around operators. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ + line = clean_lines.elided[linenum] + + # Don't try to do spacing checks for operator methods. Do this by + # replacing the troublesome characters with something else, + # preserving column position for all other characters. + # + # The replacement is done repeatedly to avoid false positives from + # operators that call operators. + while True: + match = Match(r'^(.*\boperator\b)(\S+)(\s*\(.*)$', line) + if match: + line = match.group(1) + ('_' * len(match.group(2))) + match.group(3) + else: + break # We allow no-spaces around = within an if: "if ( (a=Foo()) == 0 )". # Otherwise not. Note we only check for non-spaces on *both* sides; # sometimes people put non-spaces on one side when aligning ='s among # many lines (not that this is behavior that I approve of...) - if Search(r'[\w.]=[\w.]', line) and not Search(r'\b(if|while) ', line): + if ((Search(r'[\w.]=', line) or + Search(r'=[\w.]', line)) + and not Search(r'\b(if|while|for) ', line) + # Operators taken from [lex.operators] in C++11 standard. + and not Search(r'(>=|<=|==|!=|&=|\^=|\|=|\+=|\*=|\/=|\%=)', line) + and not Search(r'operator=', line)): error(filename, linenum, 'whitespace/operators', 4, 'Missing spaces around =') @@ -1796,26 +3853,71 @@ def CheckSpacing(filename, clean_lines, linenum, error): # though, so we punt on this one for now. TODO. # You should always have whitespace around binary operators. - # Alas, we can't test < or > because they're legitimately used sans spaces - # (a->b, vector a). The only time we can tell is a < with no >, and - # only if it's not template params list spilling into the next line. - match = Search(r'[^<>=!\s](==|!=|<=|>=)[^<>=!\s]', line) - if not match: - # Note that while it seems that the '<[^<]*' term in the following - # regexp could be simplified to '<.*', which would indeed match - # the same class of strings, the [^<] means that searching for the - # regexp takes linear rather than quadratic time. - if not Search(r'<[^<]*,\s*$', line): # template params spill - match = Search(r'[^<>=!\s](<)[^<>=!\s]([^>]|->)*$', line) + # + # Check <= and >= first to avoid false positives with < and >, then + # check non-include lines for spacing around < and >. + # + # If the operator is followed by a comma, assume it's be used in a + # macro context and don't do any checks. This avoids false + # positives. + # + # Note that && is not included here. This is because there are too + # many false positives due to RValue references. + match = Search(r'[^<>=!\s](==|!=|<=|>=|\|\|)[^<>=!\s,;\)]', line) if match: error(filename, linenum, 'whitespace/operators', 3, 'Missing spaces around %s' % match.group(1)) - # We allow no-spaces around << and >> when used like this: 10<<20, but + elif not Match(r'#.*include', line): + # Look for < that is not surrounded by spaces. This is only + # triggered if both sides are missing spaces, even though + # technically should should flag if at least one side is missing a + # space. This is done to avoid some false positives with shifts. + match = Match(r'^(.*[^\s<])<[^\s=<,]', line) + if match: + (_, _, end_pos) = CloseExpression( + clean_lines, linenum, len(match.group(1))) + if end_pos <= -1: + error(filename, linenum, 'whitespace/operators', 3, + 'Missing spaces around <') + + # Look for > that is not surrounded by spaces. Similar to the + # above, we only trigger if both sides are missing spaces to avoid + # false positives with shifts. + match = Match(r'^(.*[^-\s>])>[^\s=>,]', line) + if match: + (_, _, start_pos) = ReverseCloseExpression( + clean_lines, linenum, len(match.group(1))) + if start_pos <= -1: + error(filename, linenum, 'whitespace/operators', 3, + 'Missing spaces around >') + + # We allow no-spaces around << when used like this: 10<<20, but # not otherwise (particularly, not when used as streams) - match = Search(r'[^0-9\s](<<|>>)[^0-9\s]', line) + # + # We also allow operators following an opening parenthesis, since + # those tend to be macros that deal with operators. + match = Search(r'(operator|[^\s(<])(?:L|UL|LL|ULL|l|ul|ll|ull)?<<([^\s,=<])', line) + if (match and not (match.group(1).isdigit() and match.group(2).isdigit()) and + not (match.group(1) == 'operator' and match.group(2) == ';')): + error(filename, linenum, 'whitespace/operators', 3, + 'Missing spaces around <<') + + # We allow no-spaces around >> for almost anything. This is because + # C++11 allows ">>" to close nested templates, which accounts for + # most cases when ">>" is not followed by a space. + # + # We still warn on ">>" followed by alpha character, because that is + # likely due to ">>" being used for right shifts, e.g.: + # value >> alpha + # + # When ">>" is used to close templates, the alphanumeric letter that + # follows would be part of an identifier, and there should still be + # a space separating the template type and the identifier. + # type> alpha + match = Search(r'>>[a-zA-Z_]', line) if match: error(filename, linenum, 'whitespace/operators', 3, - 'Missing spaces around %s' % match.group(1)) + 'Missing spaces around >>') # There shouldn't be space around unary operators match = Search(r'(!\s|~\s|[\s]--[\s;]|[\s]\+\+[\s;])', line) @@ -1823,7 +3925,19 @@ def CheckSpacing(filename, clean_lines, linenum, error): error(filename, linenum, 'whitespace/operators', 4, 'Extra space for operator %s' % match.group(1)) - # A pet peeve of mine: no spaces after an if, while, switch, or for + +def CheckParenthesisSpacing(filename, clean_lines, linenum, error): + """Checks for horizontal spacing around parentheses. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ + line = clean_lines.elided[linenum] + + # No spaces after an if, while, switch, or for match = Search(r' (if\(|for\(|while\(|switch\()', line) if match: error(filename, linenum, 'whitespace/parens', 5, @@ -1844,13 +3958,36 @@ def CheckSpacing(filename, clean_lines, linenum, error): not match.group(2) and Search(r'\bfor\s*\(.*; \)', line)): error(filename, linenum, 'whitespace/parens', 5, 'Mismatching spaces inside () in %s' % match.group(1)) - if not len(match.group(2)) in [0, 1]: + if len(match.group(2)) not in [0, 1]: error(filename, linenum, 'whitespace/parens', 5, 'Should have zero or one spaces inside ( and ) in %s' % match.group(1)) + +def CheckCommaSpacing(filename, clean_lines, linenum, error): + """Checks for horizontal spacing near commas and semicolons. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ + raw = clean_lines.lines_without_raw_strings + line = clean_lines.elided[linenum] + # You should always have a space after a comma (either as fn arg or operator) - if Search(r',[^\s]', line): + # + # This does not apply when the non-space character following the + # comma is another comma, since the only time when that happens is + # for empty macro arguments. + # + # We run this check in two passes: first pass on elided lines to + # verify that lines contain missing whitespaces, second pass on raw + # lines to confirm that those missing whitespaces are not due to + # elided comments. + if (Search(r',[^,\s]', ReplaceAll(r'\boperator\s*,\s*\(', 'F(', line)) and + Search(r',[^,\s]', raw[linenum])): error(filename, linenum, 'whitespace/comma', 3, 'Missing space after ,') @@ -1862,44 +3999,175 @@ def CheckSpacing(filename, clean_lines, linenum, error): error(filename, linenum, 'whitespace/semicolon', 3, 'Missing space after ;') - # Next we will look for issues with function calls. - CheckSpacingForFunctionCall(filename, line, linenum, error) - # Except after an opening paren, or after another opening brace (in case of - # an initializer list, for instance), you should have spaces before your - # braces. And since you should never have braces at the beginning of a line, - # this is an easy test. - # if Search(r'[^ ({]{', line): - # error(filename, linenum, 'whitespace/braces', 5, - # 'Missing space before {') +def _IsType(clean_lines, nesting_state, expr): + """Check if expression looks like a type name, returns true if so. - # Make sure '} else {' has spaces. - if Search(r'}else', line): - error(filename, linenum, 'whitespace/braces', 5, - 'Missing space before else') + Args: + clean_lines: A CleansedLines instance containing the file. + nesting_state: A NestingState instance which maintains information about + the current stack of nested blocks being parsed. + expr: The expression to check. + Returns: + True, if token looks like a type. + """ + # Keep only the last token in the expression + last_word = Match(r'^.*(\b\S+)$', expr) + if last_word: + token = last_word.group(1) + else: + token = expr - # You shouldn't have spaces before your brackets, except maybe after - # 'delete []' or 'new char * []'. - if Search(r'\w\s+\[', line) and not Search(r'delete\s+\[', line): - error(filename, linenum, 'whitespace/braces', 5, - 'Extra space before [') + # Match native types and stdint types + if _TYPES.match(token): + return True - # You shouldn't have a space before a semicolon at the end of the line. - # There's a special case for "for" since the style guide allows space before - # the semicolon there. - if Search(r':\s*;\s*$', line): - error(filename, linenum, 'whitespace/semicolon', 5, - 'Semicolon defining empty statement. Use { } instead.') - elif Search(r'^\s*;\s*$', line): - error(filename, linenum, 'whitespace/semicolon', 5, - 'Line contains only semicolon. If this should be an empty statement, ' - 'use { } instead.') - elif (Search(r'\s+;\s*$', line) and - not Search(r'\bfor\b', line)): + # Try a bit harder to match templated types. Walk up the nesting + # stack until we find something that resembles a typename + # declaration for what we are looking for. + typename_pattern = (r'\b(?:typename|class|struct)\s+' + re.escape(token) + + r'\b') + block_index = len(nesting_state.stack) - 1 + while block_index >= 0: + if isinstance(nesting_state.stack[block_index], _NamespaceInfo): + return False + + # Found where the opening brace is. We want to scan from this + # line up to the beginning of the function, minus a few lines. + # template + # class C + # : public ... { // start scanning here + last_line = nesting_state.stack[block_index].starting_linenum + + next_block_start = 0 + if block_index > 0: + next_block_start = nesting_state.stack[block_index - 1].starting_linenum + first_line = last_line + while first_line >= next_block_start: + if clean_lines.elided[first_line].find('template') >= 0: + break + first_line -= 1 + if first_line < next_block_start: + # Didn't find any "template" keyword before reaching the next block, + # there are probably no template things to check for this block + block_index -= 1 + continue + + # Look for typename in the specified range + for i in xrange(first_line, last_line + 1, 1): + if Search(typename_pattern, clean_lines.elided[i]): + return True + block_index -= 1 + + return False + + +def CheckBracesSpacing(filename, clean_lines, linenum, nesting_state, error): + """Checks for horizontal spacing near commas. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + nesting_state: A NestingState instance which maintains information about + the current stack of nested blocks being parsed. + error: The function to call with any errors found. + """ + line = clean_lines.elided[linenum] + + # Except after an opening paren, or after another opening brace (in case of + # an initializer list, for instance), you should have spaces before your + # braces when they are delimiting blocks, classes, namespaces etc. + # And since you should never have braces at the beginning of a line, + # this is an easy test. Except that braces used for initialization don't + # follow the same rule; we often don't want spaces before those. + match = Match(r'^(.*[^ ({>]){', line) + + if match: + # Try a bit harder to check for brace initialization. This + # happens in one of the following forms: + # Constructor() : initializer_list_{} { ... } + # Constructor{}.MemberFunction() + # Type variable{}; + # FunctionCall(type{}, ...); + # LastArgument(..., type{}); + # LOG(INFO) << type{} << " ..."; + # map_of_type[{...}] = ...; + # ternary = expr ? new type{} : nullptr; + # OuterTemplate{}> + # + # We check for the character following the closing brace, and + # silence the warning if it's one of those listed above, i.e. + # "{.;,)<>]:". + # + # To account for nested initializer list, we allow any number of + # closing braces up to "{;,)<". We can't simply silence the + # warning on first sight of closing brace, because that would + # cause false negatives for things that are not initializer lists. + # Silence this: But not this: + # Outer{ if (...) { + # Inner{...} if (...){ // Missing space before { + # }; } + # + # There is a false negative with this approach if people inserted + # spurious semicolons, e.g. "if (cond){};", but we will catch the + # spurious semicolon with a separate check. + leading_text = match.group(1) + (endline, endlinenum, endpos) = CloseExpression( + clean_lines, linenum, len(match.group(1))) + trailing_text = '' + if endpos > -1: + trailing_text = endline[endpos:] + for offset in xrange(endlinenum + 1, + min(endlinenum + 3, clean_lines.NumLines() - 1)): + trailing_text += clean_lines.elided[offset] + # We also suppress warnings for `uint64_t{expression}` etc., as the style + # guide recommends brace initialization for integral types to avoid + # overflow/truncation. + if (not Match(r'^[\s}]*[{.;,)<>\]:]', trailing_text) + and not _IsType(clean_lines, nesting_state, leading_text)): + error(filename, linenum, 'whitespace/braces', 5, + 'Missing space before {') + + # Make sure '} else {' has spaces. + if Search(r'}else', line): + error(filename, linenum, 'whitespace/braces', 5, + 'Missing space before else') + + # You shouldn't have a space before a semicolon at the end of the line. + # There's a special case for "for" since the style guide allows space before + # the semicolon there. + if Search(r':\s*;\s*$', line): + error(filename, linenum, 'whitespace/semicolon', 5, + 'Semicolon defining empty statement. Use {} instead.') + elif Search(r'^\s*;\s*$', line): + error(filename, linenum, 'whitespace/semicolon', 5, + 'Line contains only semicolon. If this should be an empty statement, ' + 'use {} instead.') + elif (Search(r'\s+;\s*$', line) and + not Search(r'\bfor\b', line)): error(filename, linenum, 'whitespace/semicolon', 5, 'Extra space before last semicolon. If this should be an empty ' - 'statement, use { } instead.') + 'statement, use {} instead.') + + +def IsDecltype(clean_lines, linenum, column): + """Check if the token ending on (linenum, column) is decltype(). + Args: + clean_lines: A CleansedLines instance containing the file. + linenum: the number of the line to check. + column: end column of the token to check. + Returns: + True if this token is decltype() expression, False otherwise. + """ + (text, _, start_col) = ReverseCloseExpression(clean_lines, linenum, column) + if start_col < 0: + return False + if Search(r'\bdecltype\s*$', text[0:start_col]): + return True + return False def CheckSectionSpacing(filename, clean_lines, class_info, linenum, error): """Checks for additional blank line issues related to sections. @@ -1924,8 +4192,8 @@ def CheckSectionSpacing(filename, clean_lines, class_info, linenum, error): # # If we didn't find the end of the class, last_line would be zero, # and the check will be skipped by the first condition. - if (class_info.last_line - class_info.linenum <= 24 or - linenum <= class_info.linenum): + if (class_info.last_line - class_info.starting_linenum <= 24 or + linenum <= class_info.starting_linenum): return matched = Match(r'\s*(public|protected|private):', clean_lines.lines[linenum]) @@ -1936,15 +4204,18 @@ def CheckSectionSpacing(filename, clean_lines, class_info, linenum, error): # - We are at the beginning of the class. # - We are forward-declaring an inner class that is semantically # private, but needed to be public for implementation reasons. + # Also ignores cases where the previous line ends with a backslash as can be + # common when defining classes in C macros. prev_line = clean_lines.lines[linenum - 1] if (not IsBlankLine(prev_line) and - not Search(r'\b(class|struct)\b', prev_line)): + not Search(r'\b(class|struct)\b', prev_line) and + not Search(r'\\$', prev_line)): # Try a bit harder to find the beginning of the class. This is to # account for multi-line base-specifier lists, e.g.: # class Derived # : public Base { - end_class_head = class_info.linenum - for i in range(class_info.linenum, linenum): + end_class_head = class_info.starting_linenum + for i in range(class_info.starting_linenum, linenum): if Search(r'\{\s*$', clean_lines.lines[i]): end_class_head = i break @@ -1987,34 +4258,47 @@ def CheckBraces(filename, clean_lines, linenum, error): """ line = clean_lines.elided[linenum] # get rid of comments and strings - if Search(r'[\);:}]\s*{\s*$', line): - error(filename, linenum, 'whitespace/braces', 4, - '{ should never be at the end of the previous line') - # An else clause should be on the same line as the preceding closing brace. - if Match(r'\s*}\s*else\s*', line): - error(filename, linenum, 'whitespace/newline', 4, - 'An else should not appear on the same line as the previous }') + if Match(r'\s*{\s*$', line): + # We allow an open brace to start a line in the case where someone is using + # braces in a block to explicitly create a new scope, which is commonly used + # to control the lifetime of stack-allocated variables. Braces are also + # used for brace initializers inside function calls. We don't detect this + # perfectly: we just don't complain if the last non-whitespace character on + # the previous non-blank line is ',', ';', ':', '(', '{', or '}', or if the + # previous line starts a preprocessor block. We also allow a brace on the + # following line if it is part of an array initialization and would not fit + # within the 80 character limit of the preceding line. + prevline = GetPreviousNonBlankLine(clean_lines, linenum)[0] + if (not Search(r'[,;:}{(]\s*$', prevline) and + not Match(r'\s*#', prevline) and + not (GetLineWidth(prevline) > _line_length - 2 and '[]' in prevline)): + error(filename, linenum, 'whitespace/braces', 4, + '{ should almost always be at the end of the previous line') - if Match(r'\s*else\s*{', line): - error(filename, linenum, 'whitespace/newline', 4, - 'An else should not appear on the same line as the next {') + # An else clause should be on the same line as the preceding closing brace. + if Match(r'\s*else\b\s*(?:if\b|\{|$)', line): + prevline = GetPreviousNonBlankLine(clean_lines, linenum)[0] + if Match(r'\s*}\s*$', prevline): + error(filename, linenum, 'whitespace/newline', 4, + 'An else should appear on the same line as the preceding }') # If braces come on one side of an else, they should be on both. # However, we have to worry about "else if" that spans multiple lines! - if Search(r'}\s*else[^{]*$', line) or Match(r'[^}]*else\s*{', line): - if Search(r'}\s*else if([^{]*)$', line): # could be multi-line if - # find the ( after the if - pos = line.find('else if') - pos = line.find('(', pos) - if pos > 0: - (endline, _, endpos) = CloseExpression(clean_lines, linenum, pos) - if endline[endpos:].find('{') == -1: # must be brace after if - error(filename, linenum, 'readability/braces', 5, - 'If an else has a brace on one side, it should have it on both') - else: # common case: else not followed by a multi-line if - error(filename, linenum, 'readability/braces', 5, - 'If an else has a brace on one side, it should have it on both') + if Search(r'else if\s*\(', line): # could be multi-line if + brace_on_left = bool(Search(r'}\s*else if\s*\(', line)) + # find the ( after the if + pos = line.find('else if') + pos = line.find('(', pos) + if pos > 0: + (endline, _, endpos) = CloseExpression(clean_lines, linenum, pos) + brace_on_right = endline[endpos:].find('{') != -1 + if brace_on_left != brace_on_right: # must be brace after if + error(filename, linenum, 'readability/braces', 5, + 'If an else has a brace on one side, it should have it on both') + elif Search(r'}\s*else[^{]*$', line) or Match(r'[^}]*else\s*{', line): + error(filename, linenum, 'readability/braces', 5, + 'If an else has a brace on one side, it should have it on both') # Likewise, an else should never have the else clause on the same line if Search(r'\belse [^\s{]', line) and not Search(r'\belse if\b', line): @@ -2026,56 +4310,334 @@ def CheckBraces(filename, clean_lines, linenum, error): error(filename, linenum, 'whitespace/newline', 4, 'do/while clauses should not be on a single line') - # Braces shouldn't be followed by a ; unless they're defining a struct - # or initializing an array. - # We can't tell in general, but we can for some common cases. - prevlinenum = linenum - while True: - (prevline, prevlinenum) = GetPreviousNonBlankLine(clean_lines, prevlinenum) - if Match(r'\s+{.*}\s*;', line) and not prevline.count(';'): - line = prevline + line - else: - break - # if (Search(r'{.*}\s*;', line) and - # line.count('{') == line.count('}') and - # not Search(r'struct|class|enum|\s*=\s*{', line)): - # error(filename, linenum, 'readability/braces', 4, - # "You don't need a ; after a }") + # Check single-line if/else bodies. The style guide says 'curly braces are not + # required for single-line statements'. We additionally allow multi-line, + # single statements, but we reject anything with more than one semicolon in + # it. This means that the first semicolon after the if should be at the end of + # its line, and the line after that should have an indent level equal to or + # lower than the if. We also check for ambiguous if/else nesting without + # braces. + if_else_match = Search(r'\b(if\s*(|constexpr)\s*\(|else\b)', line) + if if_else_match and not Match(r'\s*#', line): + if_indent = GetIndentLevel(line) + endline, endlinenum, endpos = line, linenum, if_else_match.end() + if_match = Search(r'\bif\s*(|constexpr)\s*\(', line) + if if_match: + # This could be a multiline if condition, so find the end first. + pos = if_match.end() - 1 + (endline, endlinenum, endpos) = CloseExpression(clean_lines, linenum, pos) + # Check for an opening brace, either directly after the if or on the next + # line. If found, this isn't a single-statement conditional. + if (not Match(r'\s*{', endline[endpos:]) + and not (Match(r'\s*$', endline[endpos:]) + and endlinenum < (len(clean_lines.elided) - 1) + and Match(r'\s*{', clean_lines.elided[endlinenum + 1]))): + while (endlinenum < len(clean_lines.elided) + and ';' not in clean_lines.elided[endlinenum][endpos:]): + endlinenum += 1 + endpos = 0 + if endlinenum < len(clean_lines.elided): + endline = clean_lines.elided[endlinenum] + # We allow a mix of whitespace and closing braces (e.g. for one-liner + # methods) and a single \ after the semicolon (for macros) + endpos = endline.find(';') + if not Match(r';[\s}]*(\\?)$', endline[endpos:]): + # Semicolon isn't the last character, there's something trailing. + # Output a warning if the semicolon is not contained inside + # a lambda expression. + if not Match(r'^[^{};]*\[[^\[\]]*\][^{}]*\{[^{}]*\}\s*\)*[;,]\s*$', + endline): + error(filename, linenum, 'readability/braces', 4, + 'If/else bodies with multiple statements require braces') + elif endlinenum < len(clean_lines.elided) - 1: + # Make sure the next line is dedented + next_line = clean_lines.elided[endlinenum + 1] + next_indent = GetIndentLevel(next_line) + # With ambiguous nested if statements, this will error out on the + # if that *doesn't* match the else, regardless of whether it's the + # inner one or outer one. + if (if_match and Match(r'\s*else\b', next_line) + and next_indent != if_indent): + error(filename, linenum, 'readability/braces', 4, + 'Else clause should be indented at the same level as if. ' + 'Ambiguous nested if/else chains require braces.') + elif next_indent > if_indent: + error(filename, linenum, 'readability/braces', 4, + 'If/else bodies with multiple statements require braces') + + +def CheckTrailingSemicolon(filename, clean_lines, linenum, error): + """Looks for redundant trailing semicolon. + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ -def ReplaceableCheck(operator, macro, line): - """Determine whether a basic CHECK can be replaced with a more specific one. + line = clean_lines.elided[linenum] - For example suggest using CHECK_EQ instead of CHECK(a == b) and - similarly for CHECK_GE, CHECK_GT, CHECK_LE, CHECK_LT, CHECK_NE. + # Block bodies should not be followed by a semicolon. Due to C++11 + # brace initialization, there are more places where semicolons are + # required than not, so we explicitly list the allowed rules rather + # than listing the disallowed ones. These are the places where "};" + # should be replaced by just "}": + # 1. Some flavor of block following closing parenthesis: + # for (;;) {}; + # while (...) {}; + # switch (...) {}; + # Function(...) {}; + # if (...) {}; + # if (...) else if (...) {}; + # + # 2. else block: + # if (...) else {}; + # + # 3. const member function: + # Function(...) const {}; + # + # 4. Block following some statement: + # x = 42; + # {}; + # + # 5. Block at the beginning of a function: + # Function(...) { + # {}; + # } + # + # Note that naively checking for the preceding "{" will also match + # braces inside multi-dimensional arrays, but this is fine since + # that expression will not contain semicolons. + # + # 6. Block following another block: + # while (true) {} + # {}; + # + # 7. End of namespaces: + # namespace {}; + # + # These semicolons seems far more common than other kinds of + # redundant semicolons, possibly due to people converting classes + # to namespaces. For now we do not warn for this case. + # + # Try matching case 1 first. + match = Match(r'^(.*\)\s*)\{', line) + if match: + # Matched closing parenthesis (case 1). Check the token before the + # matching opening parenthesis, and don't warn if it looks like a + # macro. This avoids these false positives: + # - macro that defines a base class + # - multi-line macro that defines a base class + # - macro that defines the whole class-head + # + # But we still issue warnings for macros that we know are safe to + # warn, specifically: + # - TEST, TEST_F, TEST_P, MATCHER, MATCHER_P + # - TYPED_TEST + # - INTERFACE_DEF + # - EXCLUSIVE_LOCKS_REQUIRED, SHARED_LOCKS_REQUIRED, LOCKS_EXCLUDED: + # + # We implement a list of safe macros instead of a list of + # unsafe macros, even though the latter appears less frequently in + # google code and would have been easier to implement. This is because + # the downside for getting the allowed checks wrong means some extra + # semicolons, while the downside for getting disallowed checks wrong + # would result in compile errors. + # + # In addition to macros, we also don't want to warn on + # - Compound literals + # - Lambdas + # - alignas specifier with anonymous structs + # - decltype + closing_brace_pos = match.group(1).rfind(')') + opening_parenthesis = ReverseCloseExpression( + clean_lines, linenum, closing_brace_pos) + if opening_parenthesis[2] > -1: + line_prefix = opening_parenthesis[0][0:opening_parenthesis[2]] + macro = Search(r'\b([A-Z_][A-Z0-9_]*)\s*$', line_prefix) + func = Match(r'^(.*\])\s*$', line_prefix) + if ((macro and + macro.group(1) not in ( + 'TEST', 'TEST_F', 'MATCHER', 'MATCHER_P', 'TYPED_TEST', + 'EXCLUSIVE_LOCKS_REQUIRED', 'SHARED_LOCKS_REQUIRED', + 'LOCKS_EXCLUDED', 'INTERFACE_DEF')) or + (func and not Search(r'\boperator\s*\[\s*\]', func.group(1))) or + Search(r'\b(?:struct|union)\s+alignas\s*$', line_prefix) or + Search(r'\bdecltype$', line_prefix) or + Search(r'\s+=\s*$', line_prefix)): + match = None + if (match and + opening_parenthesis[1] > 1 and + Search(r'\]\s*$', clean_lines.elided[opening_parenthesis[1] - 1])): + # Multi-line lambda-expression + match = None - Args: - operator: The C++ operator used in the CHECK. - macro: The CHECK or EXPECT macro being called. - line: The current source line. + else: + # Try matching cases 2-3. + match = Match(r'^(.*(?:else|\)\s*const)\s*)\{', line) + if not match: + # Try matching cases 4-6. These are always matched on separate lines. + # + # Note that we can't simply concatenate the previous line to the + # current line and do a single match, otherwise we may output + # duplicate warnings for the blank line case: + # if (cond) { + # // blank line + # } + prevline = GetPreviousNonBlankLine(clean_lines, linenum)[0] + if prevline and Search(r'[;{}]\s*$', prevline): + match = Match(r'^(\s*)\{', line) + + # Check matching closing brace + if match: + (endline, endlinenum, endpos) = CloseExpression( + clean_lines, linenum, len(match.group(1))) + if endpos > -1 and Match(r'^\s*;', endline[endpos:]): + # Current {} pair is eligible for semicolon check, and we have found + # the redundant semicolon, output warning here. + # + # Note: because we are scanning forward for opening braces, and + # outputting warnings for the matching closing brace, if there are + # nested blocks with trailing semicolons, we will get the error + # messages in reversed order. - Returns: - True if the CHECK can be replaced with a more specific one. + # We need to check the line forward for NOLINT + raw_lines = clean_lines.raw_lines + ParseNolintSuppressions(filename, raw_lines[endlinenum-1], endlinenum-1, + error) + ParseNolintSuppressions(filename, raw_lines[endlinenum], endlinenum, + error) + + error(filename, endlinenum, 'readability/braces', 4, + "You don't need a ; after a }") + + +def CheckEmptyBlockBody(filename, clean_lines, linenum, error): + """Look for empty loop/conditional body with only a single semicolon. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. """ - # This matches decimal and hex integers, strings, and chars (in that order). - match_constant = r'([-+]?(\d+|0[xX][0-9a-fA-F]+)[lLuU]{0,3}|".*"|\'.*\')' + # Search for loop keywords at the beginning of the line. Because only + # whitespaces are allowed before the keywords, this will also ignore most + # do-while-loops, since those lines should start with closing brace. + # + # We also check "if" blocks here, since an empty conditional block + # is likely an error. + line = clean_lines.elided[linenum] + matched = Match(r'\s*(for|while|if)\s*\(', line) + if matched: + # Find the end of the conditional expression. + (end_line, end_linenum, end_pos) = CloseExpression( + clean_lines, linenum, line.find('(')) + + # Output warning if what follows the condition expression is a semicolon. + # No warning for all other cases, including whitespace or newline, since we + # have a separate check for semicolons preceded by whitespace. + if end_pos >= 0 and Match(r';', end_line[end_pos:]): + if matched.group(1) == 'if': + error(filename, end_linenum, 'whitespace/empty_conditional_body', 5, + 'Empty conditional bodies should use {}') + else: + error(filename, end_linenum, 'whitespace/empty_loop_body', 5, + 'Empty loop bodies should use {} or continue') + + # Check for if statements that have completely empty bodies (no comments) + # and no else clauses. + if end_pos >= 0 and matched.group(1) == 'if': + # Find the position of the opening { for the if statement. + # Return without logging an error if it has no brackets. + opening_linenum = end_linenum + opening_line_fragment = end_line[end_pos:] + # Loop until EOF or find anything that's not whitespace or opening {. + while not Search(r'^\s*\{', opening_line_fragment): + if Search(r'^(?!\s*$)', opening_line_fragment): + # Conditional has no brackets. + return + opening_linenum += 1 + if opening_linenum == len(clean_lines.elided): + # Couldn't find conditional's opening { or any code before EOF. + return + opening_line_fragment = clean_lines.elided[opening_linenum] + # Set opening_line (opening_line_fragment may not be entire opening line). + opening_line = clean_lines.elided[opening_linenum] + + # Find the position of the closing }. + opening_pos = opening_line_fragment.find('{') + if opening_linenum == end_linenum: + # We need to make opening_pos relative to the start of the entire line. + opening_pos += end_pos + (closing_line, closing_linenum, closing_pos) = CloseExpression( + clean_lines, opening_linenum, opening_pos) + if closing_pos < 0: + return + + # Now construct the body of the conditional. This consists of the portion + # of the opening line after the {, all lines until the closing line, + # and the portion of the closing line before the }. + if (clean_lines.raw_lines[opening_linenum] != + CleanseComments(clean_lines.raw_lines[opening_linenum])): + # Opening line ends with a comment, so conditional isn't empty. + return + if closing_linenum > opening_linenum: + # Opening line after the {. Ignore comments here since we checked above. + bodylist = list(opening_line[opening_pos+1:]) + # All lines until closing line, excluding closing line, with comments. + bodylist.extend(clean_lines.raw_lines[opening_linenum+1:closing_linenum]) + # Closing line before the }. Won't (and can't) have comments. + bodylist.append(clean_lines.elided[closing_linenum][:closing_pos-1]) + body = '\n'.join(bodylist) + else: + # If statement has brackets and fits on a single line. + body = opening_line[opening_pos+1:closing_pos-1] + + # Check if the body is empty + if not _EMPTY_CONDITIONAL_BODY_PATTERN.search(body): + return + # The body is empty. Now make sure there's not an else clause. + current_linenum = closing_linenum + current_line_fragment = closing_line[closing_pos:] + # Loop until EOF or find anything that's not whitespace or else clause. + while Search(r'^\s*$|^(?=\s*else)', current_line_fragment): + if Search(r'^(?=\s*else)', current_line_fragment): + # Found an else clause, so don't log an error. + return + current_linenum += 1 + if current_linenum == len(clean_lines.elided): + break + current_line_fragment = clean_lines.elided[current_linenum] + + # The body is empty and there's no else clause until EOF or other code. + error(filename, end_linenum, 'whitespace/empty_if_body', 4, + ('If statement had no body and no else clause')) + - # Expression to match two sides of the operator with something that - # looks like a literal, since CHECK(x == iterator) won't compile. - # This means we can't catch all the cases where a more specific - # CHECK is possible, but it's less annoying than dealing with - # extraneous warnings. - match_this = (r'\s*' + macro + r'\((\s*' + - match_constant + r'\s*' + operator + r'[^<>].*|' - r'.*[^<>]' + operator + r'\s*' + match_constant + - r'\s*\))') +def FindCheckMacro(line): + """Find a replaceable CHECK-like macro. - # Don't complain about CHECK(x == NULL) or similar because - # CHECK_EQ(x, NULL) won't compile (requires a cast). - # Also, don't complain about more complex boolean expressions - # involving && or || such as CHECK(a == b || c == d). - return Match(match_this, line) and not Search(r'NULL|&&|\|\|', line) + Args: + line: line to search on. + Returns: + (macro name, start position), or (None, -1) if no replaceable + macro is found. + """ + for macro in _CHECK_MACROS: + i = line.find(macro) + if i >= 0: + # Find opening parenthesis. Do a regular expression match here + # to make sure that we are matching the expected CHECK macro, as + # opposed to some other macro that happens to contain the CHECK + # substring. + matched = Match(r'^(.*\b' + macro + r'\s*)\(', line) + if not matched: + continue + return (macro, len(matched.group(1))) + return (None, -1) def CheckCheck(filename, clean_lines, linenum, error): @@ -2089,26 +4651,143 @@ def CheckCheck(filename, clean_lines, linenum, error): """ # Decide the set of replacement macros that should be suggested - raw_lines = clean_lines.raw_lines - current_macro = '' - for macro in _CHECK_MACROS: - if raw_lines[linenum].find(macro) >= 0: - current_macro = macro - break - if not current_macro: - # Don't waste time here if line doesn't contain 'CHECK' or 'EXPECT' + lines = clean_lines.elided + (check_macro, start_pos) = FindCheckMacro(lines[linenum]) + if not check_macro: return - line = clean_lines.elided[linenum] # get rid of comments and strings + # Find end of the boolean expression by matching parentheses + (last_line, end_line, end_pos) = CloseExpression( + clean_lines, linenum, start_pos) + if end_pos < 0: + return - # Encourage replacing plain CHECKs with CHECK_EQ/CHECK_NE/etc. - for operator in ['==', '!=', '>=', '>', '<=', '<']: - if ReplaceableCheck(operator, current_macro, line): - error(filename, linenum, 'readability/check', 2, - 'Consider using %s instead of %s(a %s b)' % ( - _CHECK_REPLACEMENT[current_macro][operator], - current_macro, operator)) - break + # If the check macro is followed by something other than a + # semicolon, assume users will log their own custom error messages + # and don't suggest any replacements. + if not Match(r'\s*;', last_line[end_pos:]): + return + + if linenum == end_line: + expression = lines[linenum][start_pos + 1:end_pos - 1] + else: + expression = lines[linenum][start_pos + 1:] + for i in xrange(linenum + 1, end_line): + expression += lines[i] + expression += last_line[0:end_pos - 1] + + # Parse expression so that we can take parentheses into account. + # This avoids false positives for inputs like "CHECK((a < 4) == b)", + # which is not replaceable by CHECK_LE. + lhs = '' + rhs = '' + operator = None + while expression: + matched = Match(r'^\s*(<<|<<=|>>|>>=|->\*|->|&&|\|\||' + r'==|!=|>=|>|<=|<|\()(.*)$', expression) + if matched: + token = matched.group(1) + if token == '(': + # Parenthesized operand + expression = matched.group(2) + (end, _) = FindEndOfExpressionInLine(expression, 0, ['(']) + if end < 0: + return # Unmatched parenthesis + lhs += '(' + expression[0:end] + expression = expression[end:] + elif token in ('&&', '||'): + # Logical and/or operators. This means the expression + # contains more than one term, for example: + # CHECK(42 < a && a < b); + # + # These are not replaceable with CHECK_LE, so bail out early. + return + elif token in ('<<', '<<=', '>>', '>>=', '->*', '->'): + # Non-relational operator + lhs += token + expression = matched.group(2) + else: + # Relational operator + operator = token + rhs = matched.group(2) + break + else: + # Unparenthesized operand. Instead of appending to lhs one character + # at a time, we do another regular expression match to consume several + # characters at once if possible. Trivial benchmark shows that this + # is more efficient when the operands are longer than a single + # character, which is generally the case. + matched = Match(r'^([^-=!<>()&|]+)(.*)$', expression) + if not matched: + matched = Match(r'^(\s*\S)(.*)$', expression) + if not matched: + break + lhs += matched.group(1) + expression = matched.group(2) + + # Only apply checks if we got all parts of the boolean expression + if not (lhs and operator and rhs): + return + + # Check that rhs do not contain logical operators. We already know + # that lhs is fine since the loop above parses out && and ||. + if rhs.find('&&') > -1 or rhs.find('||') > -1: + return + + # At least one of the operands must be a constant literal. This is + # to avoid suggesting replacements for unprintable things like + # CHECK(variable != iterator) + # + # The following pattern matches decimal, hex integers, strings, and + # characters (in that order). + lhs = lhs.strip() + rhs = rhs.strip() + match_constant = r'^([-+]?(\d+|0[xX][0-9a-fA-F]+)[lLuU]{0,3}|".*"|\'.*\')$' + if Match(match_constant, lhs) or Match(match_constant, rhs): + # Note: since we know both lhs and rhs, we can provide a more + # descriptive error message like: + # Consider using CHECK_EQ(x, 42) instead of CHECK(x == 42) + # Instead of: + # Consider using CHECK_EQ instead of CHECK(a == b) + # + # We are still keeping the less descriptive message because if lhs + # or rhs gets long, the error message might become unreadable. + error(filename, linenum, 'readability/check', 2, + 'Consider using %s instead of %s(a %s b)' % ( + _CHECK_REPLACEMENT[check_macro][operator], + check_macro, operator)) + + +def CheckAltTokens(filename, clean_lines, linenum, error): + """Check alternative keywords being used in boolean expressions. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ + line = clean_lines.elided[linenum] + + # Avoid preprocessor lines + if Match(r'^\s*#', line): + return + + # Last ditch effort to avoid multi-line comments. This will not help + # if the comment started before the current line or ended after the + # current line, but it catches most of the false positives. At least, + # it provides a way to workaround this warning for people who use + # multi-line comments in preprocessor macros. + # + # TODO(unknown): remove this once cpplint has better support for + # multi-line comments. + if line.find('/*') >= 0 or line.find('*/') >= 0: + return + + for match in _ALT_TOKEN_REPLACEMENT_PATTERN.finditer(line): + error(filename, linenum, 'readability/alt_tokens', 2, + 'Use operator %s instead of %s' % ( + _ALT_TOKEN_REPLACEMENT[match.group(1)], match.group(1))) def GetLineWidth(line): @@ -2127,13 +4806,23 @@ def GetLineWidth(line): if unicodedata.east_asian_width(uc) in ('W', 'F'): width += 2 elif not unicodedata.combining(uc): + # Issue 337 + # https://mail.python.org/pipermail/python-list/2012-August/628809.html + if (sys.version_info.major, sys.version_info.minor) <= (3, 2): + # https://github.com/python/cpython/blob/2.7/Include/unicodeobject.h#L81 + is_wide_build = sysconfig.get_config_var("Py_UNICODE_SIZE") >= 4 + # https://github.com/python/cpython/blob/2.7/Objects/unicodeobject.c#L564 + is_low_surrogate = 0xDC00 <= ord(uc) <= 0xDFFF + if not is_wide_build and is_low_surrogate: + width -= 1 + width += 1 return width else: return len(line) -def CheckStyle(filename, clean_lines, linenum, file_extension, class_state, +def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, error): """Checks rules from the 'C++ style rules' section of cppguide.html. @@ -2146,11 +4835,17 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, class_state, clean_lines: A CleansedLines instance containing the file. linenum: The number of the line to check. file_extension: The extension (without the dot) of the filename. + nesting_state: A NestingState instance which maintains information about + the current stack of nested blocks being parsed. error: The function to call with any errors found. """ - raw_lines = clean_lines.raw_lines + # Don't use "elided" lines here, otherwise we can't check commented lines. + # Don't want to use "raw" either, because we don't want to check inside C++11 + # raw strings, + raw_lines = clean_lines.lines_without_raw_strings line = raw_lines[linenum] + prev = raw_lines[linenum - 1] if linenum > 0 else '' if line.find('\t') != -1: error(filename, linenum, 'whitespace/tab', 1, @@ -2168,32 +4863,33 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, class_state, # if(match($0, " <<")) complain = 0; # if(match(prev, " +for \\(")) complain = 0; # if(prevodd && match(prevprev, " +for \\(")) complain = 0; + scope_or_label_pattern = r'\s*(?:public|private|protected|signals)(?:\s+(?:slots\s*)?)?:\s*\\?$' + classinfo = nesting_state.InnermostClass() initial_spaces = 0 cleansed_line = clean_lines.elided[linenum] while initial_spaces < len(line) and line[initial_spaces] == ' ': initial_spaces += 1 - if line and line[-1].isspace(): - error(filename, linenum, 'whitespace/end_of_line', 4, - 'Line ends in whitespace. Consider deleting these extra spaces.') - # There are certain situations we allow one space, notably for labels - elif ((initial_spaces == 1 or initial_spaces == 3) and - not Match(r'\s*\w+\s*:\s*$', cleansed_line)): + # There are certain situations we allow one space, notably for + # section labels, and also lines containing multi-line raw strings. + # We also don't check for lines that look like continuation lines + # (of lines ending in double quotes, commas, equals, or angle brackets) + # because the rules for how to indent those are non-trivial. + if (not Search(r'[",=><] *$', prev) and + (initial_spaces == 1 or initial_spaces == 3) and + not Match(scope_or_label_pattern, cleansed_line) and + not (clean_lines.raw_lines[linenum] != line and + Match(r'^\s*""', line))): error(filename, linenum, 'whitespace/indent', 3, 'Weird number of spaces at line-start. ' 'Are you using a 2-space indent?') - # Labels should always be indented at least one space. - elif not initial_spaces and line[:2] != '//' and Search(r'[^:]:\s*$', - line): - error(filename, linenum, 'whitespace/labels', 4, - 'Labels should always be indented at least one space. ' - 'If this is a member-initializer list in a constructor or ' - 'the base class list in a class definition, the colon should ' - 'be on the following line.') + if line and line[-1].isspace(): + error(filename, linenum, 'whitespace/end_of_line', 4, + 'Line ends in whitespace. Consider deleting these extra spaces.') # Check if the line is a header guard. is_header_guard = False - if file_extension == 'h': + if IsHeaderExtension(file_extension): cppvar = GetHeaderGuardCPPVariable(filename) if (line.startswith('#ifndef %s' % cppvar) or line.startswith('#define %s' % cppvar) or @@ -2207,18 +4903,23 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, class_state, # # The "$Id:...$" comment may also get very long without it being the # developers fault. + # + # Doxygen documentation copying can get pretty long when using an overloaded + # function declaration if (not line.startswith('#include') and not is_header_guard and not Match(r'^\s*//.*http(s?)://\S*$', line) and - not Match(r'^// \$Id:.*#[0-9]+ \$$', line)): + not Match(r'^\s*//\s*[^\s]*$', line) and + not Match(r'^// \$Id:.*#[0-9]+ \$$', line) and + not Match(r'^\s*/// [@\\](copydoc|copydetails|copybrief) .*$', line)): line_width = GetLineWidth(line) - if line_width > 100: - error(filename, linenum, 'whitespace/line_length', 4, - 'Lines should very rarely be longer than 100 characters') - elif line_width > 80: + if line_width > _line_length: error(filename, linenum, 'whitespace/line_length', 2, - 'Lines should be <= 80 characters long') + 'Lines should be <= %i characters long' % _line_length) if (cleansed_line.count(';') > 1 and + # allow simple single line lambdas + not Match(r'^[^{};]*\[[^\[\]]*\][^{}]*\{[^{}\n\r]*\}', + line) and # for loops are allowed two ;'s (and may run over two lines). cleansed_line.find('for') == -1 and (GetPreviousNonBlankLine(clean_lines, linenum)[0].find('for') == -1 or @@ -2227,19 +4928,26 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, class_state, not ((cleansed_line.find('case ') != -1 or cleansed_line.find('default:') != -1) and cleansed_line.find('break;') != -1)): - error(filename, linenum, 'whitespace/newline', 4, + error(filename, linenum, 'whitespace/newline', 0, 'More than one command on the same line') # Some more style checks CheckBraces(filename, clean_lines, linenum, error) - CheckSpacing(filename, clean_lines, linenum, error) + CheckTrailingSemicolon(filename, clean_lines, linenum, error) + CheckEmptyBlockBody(filename, clean_lines, linenum, error) + CheckSpacing(filename, clean_lines, linenum, nesting_state, error) + CheckOperatorSpacing(filename, clean_lines, linenum, error) + CheckParenthesisSpacing(filename, clean_lines, linenum, error) + CheckCommaSpacing(filename, clean_lines, linenum, error) + CheckBracesSpacing(filename, clean_lines, linenum, nesting_state, error) + CheckSpacingForFunctionCall(filename, clean_lines, linenum, error) CheckCheck(filename, clean_lines, linenum, error) - if class_state and class_state.classinfo_stack: - CheckSectionSpacing(filename, clean_lines, - class_state.classinfo_stack[-1], linenum, error) + CheckAltTokens(filename, clean_lines, linenum, error) + classinfo = nesting_state.InnermostClass() + if classinfo: + CheckSectionSpacing(filename, clean_lines, classinfo, linenum, error) -_RE_PATTERN_INCLUDE_NEW_STYLE = re.compile(r'#include +"[^/]+\.h"') _RE_PATTERN_INCLUDE = re.compile(r'^\s*#\s*include\s*([<"])([^>"]*)[>"].*$') # Matches the first component of a filename delimited by -s and _s. That is: # _RE_FIRST_COMPONENT.match('foo').group(0) == 'foo' @@ -2268,38 +4976,25 @@ def _DropCommonSuffixes(filename): Returns: The filename with the common suffix removed. """ - for suffix in ('test.cc', 'regtest.cc', 'unittest.cc', - 'inl.h', 'impl.h', 'internal.h'): + for suffix in itertools.chain( + ('%s.%s' % (test_suffix.lstrip('_'), ext) + for test_suffix, ext in itertools.product(_test_suffixes, GetNonHeaderExtensions())), + ('%s.%s' % (suffix, ext) + for suffix, ext in itertools.product(['inl', 'imp', 'internal'], GetHeaderExtensions()))): if (filename.endswith(suffix) and len(filename) > len(suffix) and filename[-len(suffix) - 1] in ('-', '_')): return filename[:-len(suffix) - 1] return os.path.splitext(filename)[0] -def _IsTestFilename(filename): - """Determines if the given filename has a suffix that identifies it as a test. - - Args: - filename: The input filename. - - Returns: - True if 'filename' looks like a test, False otherwise. - """ - if (filename.endswith('_test.cc') or - filename.endswith('_unittest.cc') or - filename.endswith('_regtest.cc')): - return True - else: - return False - - -def _ClassifyInclude(fileinfo, include, is_system): +def _ClassifyInclude(fileinfo, include, used_angle_brackets, include_order="default"): """Figures out what kind of header 'include' is. Args: fileinfo: The current file cpplint is running over. A FileInfo instance. include: The path to a #included file. - is_system: True if the #include used <> rather than "". + used_angle_brackets: True if the #include used <> rather than "". + include_order: "default" or other value allowed in program arguments Returns: One of the _XXX_HEADER constants. @@ -2309,6 +5004,8 @@ def _ClassifyInclude(fileinfo, include, is_system): _C_SYS_HEADER >>> _ClassifyInclude(FileInfo('foo/foo.cc'), 'string', True) _CPP_SYS_HEADER + >>> _ClassifyInclude(FileInfo('foo/foo.cc'), 'foo/foo.h', True, "standardcfirst") + _OTHER_SYS_HEADER >>> _ClassifyInclude(FileInfo('foo/foo.cc'), 'foo/foo.h', False) _LIKELY_MY_HEADER >>> _ClassifyInclude(FileInfo('foo/foo_unknown_extension.cc'), @@ -2319,14 +5016,23 @@ def _ClassifyInclude(fileinfo, include, is_system): """ # This is a list of all standard c++ header files, except # those already checked for above. - is_stl_h = include in _STL_HEADERS - is_cpp_h = is_stl_h or include in _CPP_HEADERS or include.find(".hpp") or include.find(".hh") > 0 + is_cpp_header = include in _CPP_HEADERS + + # Mark include as C header if in list or in a known folder for standard-ish C headers. + is_std_c_header = (include_order == "default") or (include in _C_HEADERS + # additional linux glibc header folders + or Search(r'(?:%s)\/.*\.h' % "|".join(C_STANDARD_HEADER_FOLDERS), include)) + + # Headers with C++ extensions shouldn't be considered C system headers + is_system = used_angle_brackets and not os.path.splitext(include)[1] in ['.hpp', '.hxx', '.h++'] if is_system: - if is_cpp_h: + if is_cpp_header: return _CPP_SYS_HEADER - else: + if is_std_c_header: return _C_SYS_HEADER + else: + return _OTHER_SYS_HEADER # If the target file and the include we're checking share a # basename when we drop common extensions, and the include @@ -2334,9 +5040,11 @@ def _ClassifyInclude(fileinfo, include, is_system): target_dir, target_base = ( os.path.split(_DropCommonSuffixes(fileinfo.RepositoryName()))) include_dir, include_base = os.path.split(_DropCommonSuffixes(include)) + target_dir_pub = os.path.normpath(target_dir + '/../public') + target_dir_pub = target_dir_pub.replace('\\', '/') if target_base == include_base and ( include_dir == target_dir or - include_dir == os.path.normpath(target_dir + '/../public')): + include_dir == target_dir_pub): return _LIKELY_MY_HEADER # If the target and include share some initial basename @@ -2369,13 +5077,19 @@ def CheckIncludeLine(filename, clean_lines, linenum, include_state, error): error: The function to call with any errors found. """ fileinfo = FileInfo(filename) - line = clean_lines.lines[linenum] # "include" should use the new style "foo/bar.h" instead of just "bar.h" - #if _RE_PATTERN_INCLUDE_NEW_STYLE.search(line): - # error(filename, linenum, 'build/include', 4, - # 'Include the directory when naming .h files') + # Only do this check if the included header follows google naming + # conventions. If not, assume that it's a 3rd party API that + # requires special include conventions. + # + # We also make an exception for Lua headers, which follow google + # naming convention but not the include convention. + match = Match(r'#include\s*"([^/]+\.h)"', line) + if match and not _THIRD_PARTY_HEADERS_PATTERN.match(match.group(1)): + error(filename, linenum, 'build/include_subdir', 4, + 'Include the directory when naming .h files') # we shouldn't include a file more than once. actually, there are a # handful of instances where doing so is okay, but in general it's @@ -2383,13 +5097,35 @@ def CheckIncludeLine(filename, clean_lines, linenum, include_state, error): match = _RE_PATTERN_INCLUDE.search(line) if match: include = match.group(2) - is_system = (match.group(1) == '<') - if include in include_state: + used_angle_brackets = (match.group(1) == '<') + duplicate_line = include_state.FindHeader(include) + if duplicate_line >= 0: error(filename, linenum, 'build/include', 4, '"%s" already included at %s:%s' % - (include, filename, include_state[include])) - else: - include_state[include] = linenum + (include, filename, duplicate_line)) + return + + for extension in GetNonHeaderExtensions(): + if (include.endswith('.' + extension) and + os.path.dirname(fileinfo.RepositoryName()) != os.path.dirname(include)): + error(filename, linenum, 'build/include', 4, + 'Do not include .' + extension + ' files from other packages') + return + + # We DO want to include a 3rd party looking header if it matches the + # filename. Otherwise we get an erroneous error "...should include its + # header" error later. + third_src_header = False + for ext in GetHeaderExtensions(): + basefilename = filename[0:len(filename) - len(fileinfo.Extension())] + headerfile = basefilename + '.' + ext + headername = FileInfo(headerfile).RepositoryName() + if headername in include or include in headername: + third_src_header = True + break + + if third_src_header or not _THIRD_PARTY_HEADERS_PATTERN.match(include): + include_state.include_list[-1].append((include, linenum)) # We want to ensure that headers appear in the right order: # 1) for foo.cc, foo.h (preferred location) @@ -2403,28 +5139,22 @@ def CheckIncludeLine(filename, clean_lines, linenum, include_state, error): # track of the highest type seen, and complains if we see a # lower type after that. error_message = include_state.CheckNextIncludeOrder( - _ClassifyInclude(fileinfo, include, is_system)) + _ClassifyInclude(fileinfo, include, used_angle_brackets, _include_order)) if error_message: error(filename, linenum, 'build/include_order', 4, - '%s. Should be: c system, c++ system, other, %s.h .' % + '%s. Should be: %s.h, c system, c++ system, other.' % (error_message, fileinfo.BaseName())) - if not include_state.IsInAlphabeticalOrder(include): + canonical_include = include_state.CanonicalizeAlphabeticalOrder(include) + if not include_state.IsInAlphabeticalOrder( + clean_lines, linenum, canonical_include): error(filename, linenum, 'build/include_alpha', 4, 'Include "%s" not in alphabetical order' % include) + include_state.SetLastHeader(canonical_include) - # Look for any of the stream classes that are part of standard C++. - #match = _RE_PATTERN_INCLUDE.match(line) - #if match: - # include = match.group(2) - # if Match(r'(f|ind|io|i|o|parse|pf|stdio|str|)?stream$', include): - # # Many unit tests use cout, so we exempt them. - # if not _IsTestFilename(filename): - # error(filename, linenum, 'readability/streams', 3, - # 'Streams are highly discouraged.') def _GetTextInside(text, start_pattern): - """Retrieves all the text between matching open and close parentheses. + r"""Retrieves all the text between matching open and close parentheses. Given a string of lines and a regular expression string, retrieve all the text following the expression and between opening punctuation symbols like @@ -2443,12 +5173,12 @@ def _GetTextInside(text, start_pattern): The extracted text. None if either the opening string or ending punctuation could not be found. """ - # TODO(sugawarayu): Audit cpplint.py to see what places could be profitably + # TODO(unknown): Audit cpplint.py to see what places could be profitably # rewritten to use _GetTextInside (and use inferior regexp matching today). # Give opening punctuations to get the matching close-punctuations. matching_punctuation = {'(': ')', '{': '}', '[': ']'} - closing_punctuation = set(matching_punctuation.itervalues()) + closing_punctuation = set(itervalues(matching_punctuation)) # Find the position to start extracting text. match = re.search(start_pattern, text, re.M) @@ -2479,8 +5209,37 @@ def _GetTextInside(text, start_pattern): return text[start_position:position - 1] -def CheckLanguage(filename, clean_lines, linenum, file_extension, include_state, - error): +# Patterns for matching call-by-reference parameters. +# +# Supports nested templates up to 2 levels deep using this messy pattern: +# < (?: < (?: < [^<>]* +# > +# | [^<>] )* +# > +# | [^<>] )* +# > +_RE_PATTERN_IDENT = r'[_a-zA-Z]\w*' # =~ [[:alpha:]][[:alnum:]]* +_RE_PATTERN_TYPE = ( + r'(?:const\s+)?(?:typename\s+|class\s+|struct\s+|union\s+|enum\s+)?' + r'(?:\w|' + r'\s*<(?:<(?:<[^<>]*>|[^<>])*>|[^<>])*>|' + r'::)+') +# A call-by-reference parameter ends with '& identifier'. +_RE_PATTERN_REF_PARAM = re.compile( + r'(' + _RE_PATTERN_TYPE + r'(?:\s*(?:\bconst\b|[*]))*\s*' + r'&\s*' + _RE_PATTERN_IDENT + r')\s*(?:=[^,()]+)?[,)]') +# A call-by-const-reference parameter either ends with 'const& identifier' +# or looks like 'const type& identifier' when 'type' is atomic. +_RE_PATTERN_CONST_REF_PARAM = ( + r'(?:.*\s*\bconst\s*&\s*' + _RE_PATTERN_IDENT + + r'|const\s+' + _RE_PATTERN_TYPE + r'\s*&\s*' + _RE_PATTERN_IDENT + r')') +# Stream types. +_RE_PATTERN_REF_STREAM_PARAM = ( + r'(?:.*stream\s*&\s*' + _RE_PATTERN_IDENT + r')') + + +def CheckLanguage(filename, clean_lines, linenum, file_extension, + include_state, nesting_state, error): """Checks rules from the 'C++ language rules' section of cppguide.html. Some of these rules are hard to test (function overloading, using @@ -2492,6 +5251,8 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, include_state, linenum: The number of the line to check. file_extension: The extension (without the dot) of the filename. include_state: An _IncludeState instance in which the headers are inserted. + nesting_state: A NestingState instance which maintains information about + the current stack of nested blocks being parsed. error: The function to call with any errors found. """ # If the line is empty or consists of entirely a comment, no need to @@ -2505,121 +5266,23 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, include_state, CheckIncludeLine(filename, clean_lines, linenum, include_state, error) return - # Create an extended_line, which is the concatenation of the current and - # next lines, for more effective checking of code that may span more than one - # line. - if linenum + 1 < clean_lines.NumLines(): - extended_line = line + clean_lines.elided[linenum + 1] - else: - extended_line = line - - # Make Windows paths like Unix. - fullname = os.path.abspath(filename).replace('\\', '/') - - # TODO(unknown): figure out if they're using default arguments in fn proto. - - # Check for non-const references in functions. This is tricky because & - # is also used to take the address of something. We allow <> for templates, - # (ignoring whatever is between the braces) and : for classes. - # These are complicated re's. They try to capture the following: - # paren (for fn-prototype start), typename, &, varname. For the const - # version, we're willing for const to be before typename or after - # Don't check the implementation on same line. - #fnline = line.split('{', 1)[0] - #if (len(re.findall(r'\([^()]*\b(?:[\w:]|<[^()]*>)+(\s?&|&\s?)\w+', fnline)) > - # len(re.findall(r'\([^()]*\bconst\s+(?:typename\s+)?(?:struct\s+)?' - # r'(?:[\w:]|<[^()]*>)+(\s?&|&\s?)\w+', fnline)) + - # len(re.findall(r'\([^()]*\b(?:[\w:]|<[^()]*>)+\s+const(\s?&|&\s?)[\w]+', - # fnline))): - - # # We allow non-const references in a few standard places, like functions - # # called "swap()" or iostream operators like "<<" or ">>". - # if not Search( - # r'(swap|Swap|operator[<>][<>])\s*\(\s*(?:[\w:]|<.*>)+\s*&', - # fnline): - # error(filename, linenum, 'runtime/references', 2, - # 'Is this a non-const reference? ' - # 'If so, make const or use a pointer.') - - # Check to see if they're using an conversion function cast. - # I just try to capture the most common basic types, though there are more. - # Parameterless conversion functions, such as bool(), are allowed as they are - # probably a member operator declaration or default constructor. - match = Search( - r'(\bnew\s+)?\b' # Grab 'new' operator, if it's there - r'(int|float|double|bool|char|int32|uint32|int64|uint64)\([^)]', line) + # Reset include state across preprocessor directives. This is meant + # to silence warnings for conditional includes. + match = Match(r'^\s*#\s*(if|ifdef|ifndef|elif|else|endif)\b', line) if match: - # gMock methods are defined using some variant of MOCK_METHODx(name, type) - # where type may be float(), int(string), etc. Without context they are - # virtually indistinguishable from int(x) casts. Likewise, gMock's - # MockCallback takes a template parameter of the form return_type(arg_type), - # which looks much like the cast we're trying to detect. - if (match.group(1) is None and # If new operator, then this isn't a cast - not (Match(r'^\s*MOCK_(CONST_)?METHOD\d+(_T)?\(', line) or - Match(r'^\s*MockCallback<.*>', line))): - error(filename, linenum, 'readability/casting', 4, - 'Using deprecated casting style. ' - 'Use static_cast<%s>(...) instead' % - match.group(2)) + include_state.ResetSection(match.group(1)) - CheckCStyleCast(filename, linenum, line, clean_lines.raw_lines[linenum], - 'static_cast', - r'\((int|float|double|bool|char|u?int(16|32|64))\)', error) - # This doesn't catch all cases. Consider (const char * const)"hello". - # - # (char *) "foo" should always be a const_cast (reinterpret_cast won't - # compile). - if CheckCStyleCast(filename, linenum, line, clean_lines.raw_lines[linenum], - 'const_cast', r'\((char\s?\*+\s?)\)\s*"', error): - pass - else: - # Check pointer casts for other than string constants - CheckCStyleCast(filename, linenum, line, clean_lines.raw_lines[linenum], - 'reinterpret_cast', r'\((\w+\s?\*+\s?)\)', error) - - # In addition, we look for people taking the address of a cast. This - # is dangerous -- casts can assign to temporaries, so the pointer doesn't - # point where you think. - if Search( - r'(&\([^)]+\)[\w(])|(&(static|dynamic|reinterpret)_cast\b)', line): - error(filename, linenum, 'runtime/casting', 4, - ('Are you taking an address of a cast? ' - 'This is dangerous: could be a temp var. ' - 'Take the address before doing the cast, rather than after')) - - # Check for people declaring static/global STL strings at the top level. - # This is dangerous because the C++ language does not guarantee that - # globals with constructors are initialized before the first access. - match = Match( - r'((?:|static +)(?:|const +))string +([a-zA-Z0-9_:]+)\b(.*)', - line) - # Make sure it's not a function. - # Function template specialization looks like: "string foo(...". - # Class template definitions look like: "string Foo::Method(...". - if match and not Match(r'\s*(<.*>)?(::[a-zA-Z0-9_]+)?\s*\(([^"]|$)', - match.group(3)): - error(filename, linenum, 'runtime/string', 4, - 'For a static/global string constant, use a C style string instead: ' - '"%schar %s[]".' % - (match.group(1), match.group(2))) - - # Check that we're not using RTTI outside of testing code. - #if Search(r'\bdynamic_cast<', line) and not _IsTestFilename(filename): - # error(filename, linenum, 'runtime/rtti', 5, - # 'Do not use dynamic_cast<>. If you need to cast within a class ' - # "hierarchy, use static_cast<> to upcast. Google doesn't support " - # 'RTTI.') - - if Search(r'\b([A-Za-z0-9_]*_)\(\1\)', line): - error(filename, linenum, 'runtime/init', 4, - 'You seem to be initializing a member variable with itself.') + # Perform other checks now that we are sure that this is not an include line + CheckCasts(filename, clean_lines, linenum, error) + CheckGlobalStatic(filename, clean_lines, linenum, error) + CheckPrintf(filename, clean_lines, linenum, error) - if file_extension == 'h': + if IsHeaderExtension(file_extension): # TODO(unknown): check that 1-arg constructors are explicit. # How to tell it's a constructor? # (handled in CheckForNonStandardConstructs for now) - # TODO(unknown): check that classes have DISALLOW_EVIL_CONSTRUCTORS + # TODO(unknown): check that classes declare or disable copy/assign # (level 1 error) pass @@ -2635,27 +5298,6 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, include_state, error(filename, linenum, 'runtime/int', 4, 'Use int16/int64/etc, rather than the C type %s' % match.group(1)) - # When snprintf is used, the second argument shouldn't be a literal. - match = Search(r'snprintf\s*\(([^,]*),\s*([0-9]*)\s*,', line) - if match and match.group(2) != '0': - # If 2nd arg is zero, snprintf is used to calculate size. - error(filename, linenum, 'runtime/printf', 3, - 'If you can, use sizeof(%s) instead of %s as the 2nd arg ' - 'to snprintf.' % (match.group(1), match.group(2))) - - # Check if some verboten C functions are being used. - if Search(r'\bsprintf\b', line): - error(filename, linenum, 'runtime/printf', 5, - 'Never use sprintf. Use snprintf instead.') - match = Search(r'\b(strcpy|strcat)\b', line) - if match: - error(filename, linenum, 'runtime/printf', 4, - 'Almost always, snprintf is better than %s' % match.group(1)) - - #if Search(r'\bsscanf\b', line): - # error(filename, linenum, 'runtime/printf', 1, - # 'sscanf can be ok, but is slow and can overflow buffers.') - # Check if some verboten operator overloading is going on # TODO(unknown): catch out-of-line unary operator&: # class X {}; @@ -2675,114 +5317,523 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, include_state, # Check for potential format string bugs like printf(foo). # We constrain the pattern not to pick things like DocidForPrintf(foo). # Not perfect but it can catch printf(foo.c_str()) and printf(foo->c_str()) - # TODO(sugawarayu): Catch the following case. Need to change the calling + # TODO(unknown): Catch the following case. Need to change the calling # convention of the whole function to process multiple line to handle it. # printf( # boy_this_is_a_really_long_variable_that_cannot_fit_on_the_prev_line); printf_args = _GetTextInside(line, r'(?i)\b(string)?printf\s*\(') if printf_args: match = Match(r'([\w.\->()]+)$', printf_args) - if match: + if match and match.group(1) != '__VA_ARGS__': function_name = re.search(r'\b((?:string)?printf)\s*\(', line, re.I).group(1) error(filename, linenum, 'runtime/printf', 4, 'Potential format string bug. Do %s("%%s", %s) instead.' % (function_name, match.group(1))) - # Check for potential memset bugs like memset(buf, sizeof(buf), 0). - match = Search(r'memset\s*\(([^,]*),\s*([^,]*),\s*0\s*\)', line) - if match and not Match(r"^''|-?[0-9]+|0x[0-9A-Fa-f]$", match.group(2)): - error(filename, linenum, 'runtime/memset', 4, - 'Did you mean "memset(%s, 0, %s)"?' - % (match.group(1), match.group(2))) + # Check for potential memset bugs like memset(buf, sizeof(buf), 0). + match = Search(r'memset\s*\(([^,]*),\s*([^,]*),\s*0\s*\)', line) + if match and not Match(r"^''|-?[0-9]+|0x[0-9A-Fa-f]$", match.group(2)): + error(filename, linenum, 'runtime/memset', 4, + 'Did you mean "memset(%s, 0, %s)"?' + % (match.group(1), match.group(2))) + + if Search(r'\busing namespace\b', line): + if Search(r'\bliterals\b', line): + error(filename, linenum, 'build/namespaces_literals', 5, + 'Do not use namespace using-directives. ' + 'Use using-declarations instead.') + else: + error(filename, linenum, 'build/namespaces', 5, + 'Do not use namespace using-directives. ' + 'Use using-declarations instead.') + + # Detect variable-length arrays. + match = Match(r'\s*(.+::)?(\w+) [a-z]\w*\[(.+)];', line) + if (match and match.group(2) != 'return' and match.group(2) != 'delete' and + match.group(3).find(']') == -1): + # Split the size using space and arithmetic operators as delimiters. + # If any of the resulting tokens are not compile time constants then + # report the error. + tokens = re.split(r'\s|\+|\-|\*|\/|<<|>>]', match.group(3)) + is_const = True + skip_next = False + for tok in tokens: + if skip_next: + skip_next = False + continue + + if Search(r'sizeof\(.+\)', tok): continue + if Search(r'arraysize\(\w+\)', tok): continue + + tok = tok.lstrip('(') + tok = tok.rstrip(')') + if not tok: continue + if Match(r'\d+', tok): continue + if Match(r'0[xX][0-9a-fA-F]+', tok): continue + if Match(r'k[A-Z0-9]\w*', tok): continue + if Match(r'(.+::)?k[A-Z0-9]\w*', tok): continue + if Match(r'(.+::)?[A-Z][A-Z0-9_]*', tok): continue + # A catch all for tricky sizeof cases, including 'sizeof expression', + # 'sizeof(*type)', 'sizeof(const type)', 'sizeof(struct StructName)' + # requires skipping the next token because we split on ' ' and '*'. + if tok.startswith('sizeof'): + skip_next = True + continue + is_const = False + break + if not is_const: + error(filename, linenum, 'runtime/arrays', 1, + 'Do not use variable-length arrays. Use an appropriately named ' + "('k' followed by CamelCase) compile-time constant for the size.") + + # Check for use of unnamed namespaces in header files. Registration + # macros are typically OK, so we allow use of "namespace {" on lines + # that end with backslashes. + if (IsHeaderExtension(file_extension) + and Search(r'\bnamespace\s*{', line) + and line[-1] != '\\'): + error(filename, linenum, 'build/namespaces_headers', 4, + 'Do not use unnamed namespaces in header files. See ' + 'https://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces' + ' for more information.') + + +def CheckGlobalStatic(filename, clean_lines, linenum, error): + """Check for unsafe global or static objects. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ + line = clean_lines.elided[linenum] + + # Match two lines at a time to support multiline declarations + if linenum + 1 < clean_lines.NumLines() and not Search(r'[;({]', line): + line += clean_lines.elided[linenum + 1].strip() + + # Check for people declaring static/global STL strings at the top level. + # This is dangerous because the C++ language does not guarantee that + # globals with constructors are initialized before the first access, and + # also because globals can be destroyed when some threads are still running. + # TODO(unknown): Generalize this to also find static unique_ptr instances. + # TODO(unknown): File bugs for clang-tidy to find these. + match = Match( + r'((?:|static +)(?:|const +))(?::*std::)?string( +const)? +' + r'([a-zA-Z0-9_:]+)\b(.*)', + line) + + # Remove false positives: + # - String pointers (as opposed to values). + # string *pointer + # const string *pointer + # string const *pointer + # string *const pointer + # + # - Functions and template specializations. + # string Function(... + # string Class::Method(... + # + # - Operators. These are matched separately because operator names + # cross non-word boundaries, and trying to match both operators + # and functions at the same time would decrease accuracy of + # matching identifiers. + # string Class::operator*() + if (match and + not Search(r'\bstring\b(\s+const)?\s*[\*\&]\s*(const\s+)?\w', line) and + not Search(r'\boperator\W', line) and + not Match(r'\s*(<.*>)?(::[a-zA-Z0-9_]+)*\s*\(([^"]|$)', match.group(4))): + if Search(r'\bconst\b', line): + error(filename, linenum, 'runtime/string', 4, + 'For a static/global string constant, use a C style string ' + 'instead: "%schar%s %s[]".' % + (match.group(1), match.group(2) or '', match.group(3))) + else: + error(filename, linenum, 'runtime/string', 4, + 'Static/global string variables are not permitted.') + + if (Search(r'\b([A-Za-z0-9_]*_)\(\1\)', line) or + Search(r'\b([A-Za-z0-9_]*_)\(CHECK_NOTNULL\(\1\)\)', line)): + error(filename, linenum, 'runtime/init', 4, + 'You seem to be initializing a member variable with itself.') + + +def CheckPrintf(filename, clean_lines, linenum, error): + """Check for printf related issues. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ + line = clean_lines.elided[linenum] + + # When snprintf is used, the second argument shouldn't be a literal. + match = Search(r'snprintf\s*\(([^,]*),\s*([0-9]*)\s*,', line) + if match and match.group(2) != '0': + # If 2nd arg is zero, snprintf is used to calculate size. + error(filename, linenum, 'runtime/printf', 3, + 'If you can, use sizeof(%s) instead of %s as the 2nd arg ' + 'to snprintf.' % (match.group(1), match.group(2))) + + # Check if some verboten C functions are being used. + if Search(r'\bsprintf\s*\(', line): + error(filename, linenum, 'runtime/printf', 5, + 'Never use sprintf. Use snprintf instead.') + match = Search(r'\b(strcpy|strcat)\s*\(', line) + if match: + error(filename, linenum, 'runtime/printf', 4, + 'Almost always, snprintf is better than %s' % match.group(1)) + + +def IsDerivedFunction(clean_lines, linenum): + """Check if current line contains an inherited function. + + Args: + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + Returns: + True if current line contains a function with "override" + virt-specifier. + """ + # Scan back a few lines for start of current function + for i in xrange(linenum, max(-1, linenum - 10), -1): + match = Match(r'^([^()]*\w+)\(', clean_lines.elided[i]) + if match: + # Look for "override" after the matching closing parenthesis + line, _, closing_paren = CloseExpression( + clean_lines, i, len(match.group(1))) + return (closing_paren >= 0 and + Search(r'\boverride\b', line[closing_paren:])) + return False + + +def IsOutOfLineMethodDefinition(clean_lines, linenum): + """Check if current line contains an out-of-line method definition. + + Args: + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + Returns: + True if current line contains an out-of-line method definition. + """ + # Scan back a few lines for start of current function + for i in xrange(linenum, max(-1, linenum - 10), -1): + if Match(r'^([^()]*\w+)\(', clean_lines.elided[i]): + return Match(r'^[^()]*\w+::\w+\(', clean_lines.elided[i]) is not None + return False + + +def IsInitializerList(clean_lines, linenum): + """Check if current line is inside constructor initializer list. + + Args: + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + Returns: + True if current line appears to be inside constructor initializer + list, False otherwise. + """ + for i in xrange(linenum, 1, -1): + line = clean_lines.elided[i] + if i == linenum: + remove_function_body = Match(r'^(.*)\{\s*$', line) + if remove_function_body: + line = remove_function_body.group(1) + + if Search(r'\s:\s*\w+[({]', line): + # A lone colon tend to indicate the start of a constructor + # initializer list. It could also be a ternary operator, which + # also tend to appear in constructor initializer lists as + # opposed to parameter lists. + return True + if Search(r'\}\s*,\s*$', line): + # A closing brace followed by a comma is probably the end of a + # brace-initialized member in constructor initializer list. + return True + if Search(r'[{};]\s*$', line): + # Found one of the following: + # - A closing brace or semicolon, probably the end of the previous + # function. + # - An opening brace, probably the start of current class or namespace. + # + # Current line is probably not inside an initializer list since + # we saw one of those things without seeing the starting colon. + return False + + # Got to the beginning of the file without seeing the start of + # constructor initializer list. + return False + + +def CheckForNonConstReference(filename, clean_lines, linenum, + nesting_state, error): + """Check for non-const references. + + Separate from CheckLanguage since it scans backwards from current + line, instead of scanning forward. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + nesting_state: A NestingState instance which maintains information about + the current stack of nested blocks being parsed. + error: The function to call with any errors found. + """ + # Do nothing if there is no '&' on current line. + line = clean_lines.elided[linenum] + if '&' not in line: + return + + # If a function is inherited, current function doesn't have much of + # a choice, so any non-const references should not be blamed on + # derived function. + if IsDerivedFunction(clean_lines, linenum): + return + + # Don't warn on out-of-line method definitions, as we would warn on the + # in-line declaration, if it isn't marked with 'override'. + if IsOutOfLineMethodDefinition(clean_lines, linenum): + return + + # Long type names may be broken across multiple lines, usually in one + # of these forms: + # LongType + # ::LongTypeContinued &identifier + # LongType:: + # LongTypeContinued &identifier + # LongType< + # ...>::LongTypeContinued &identifier + # + # If we detected a type split across two lines, join the previous + # line to current line so that we can match const references + # accordingly. + # + # Note that this only scans back one line, since scanning back + # arbitrary number of lines would be expensive. If you have a type + # that spans more than 2 lines, please use a typedef. + if linenum > 1: + previous = None + if Match(r'\s*::(?:[\w<>]|::)+\s*&\s*\S', line): + # previous_line\n + ::current_line + previous = Search(r'\b((?:const\s*)?(?:[\w<>]|::)+[\w<>])\s*$', + clean_lines.elided[linenum - 1]) + elif Match(r'\s*[a-zA-Z_]([\w<>]|::)+\s*&\s*\S', line): + # previous_line::\n + current_line + previous = Search(r'\b((?:const\s*)?(?:[\w<>]|::)+::)\s*$', + clean_lines.elided[linenum - 1]) + if previous: + line = previous.group(1) + line.lstrip() + else: + # Check for templated parameter that is split across multiple lines + endpos = line.rfind('>') + if endpos > -1: + (_, startline, startpos) = ReverseCloseExpression( + clean_lines, linenum, endpos) + if startpos > -1 and startline < linenum: + # Found the matching < on an earlier line, collect all + # pieces up to current line. + line = '' + for i in xrange(startline, linenum + 1): + line += clean_lines.elided[i].strip() + + # Check for non-const references in function parameters. A single '&' may + # found in the following places: + # inside expression: binary & for bitwise AND + # inside expression: unary & for taking the address of something + # inside declarators: reference parameter + # We will exclude the first two cases by checking that we are not inside a + # function body, including one that was just introduced by a trailing '{'. + # TODO(unknown): Doesn't account for 'catch(Exception& e)' [rare]. + if (nesting_state.previous_stack_top and + not (isinstance(nesting_state.previous_stack_top, _ClassInfo) or + isinstance(nesting_state.previous_stack_top, _NamespaceInfo))): + # Not at toplevel, not within a class, and not within a namespace + return + + # Avoid initializer lists. We only need to scan back from the + # current line for something that starts with ':'. + # + # We don't need to check the current line, since the '&' would + # appear inside the second set of parentheses on the current line as + # opposed to the first set. + if linenum > 0: + for i in xrange(linenum - 1, max(0, linenum - 10), -1): + previous_line = clean_lines.elided[i] + if not Search(r'[),]\s*$', previous_line): + break + if Match(r'^\s*:\s+\S', previous_line): + return + + # Avoid preprocessors + if Search(r'\\\s*$', line): + return + + # Avoid constructor initializer lists + if IsInitializerList(clean_lines, linenum): + return + + # We allow non-const references in a few standard places, like functions + # called "swap()" or iostream operators like "<<" or ">>". Do not check + # those function parameters. + # + # We also accept & in static_assert, which looks like a function but + # it's actually a declaration expression. + allowed_functions = (r'(?:[sS]wap(?:<\w:+>)?|' + r'operator\s*[<>][<>]|' + r'static_assert|COMPILE_ASSERT' + r')\s*\(') + if Search(allowed_functions, line): + return + elif not Search(r'\S+\([^)]*$', line): + # Don't see an allowed function on this line. Actually we + # didn't see any function name on this line, so this is likely a + # multi-line parameter list. Try a bit harder to catch this case. + for i in xrange(2): + if (linenum > i and + Search(allowed_functions, clean_lines.elided[linenum - i - 1])): + return + + decls = ReplaceAll(r'{[^}]*}', ' ', line) # exclude function body + for parameter in re.findall(_RE_PATTERN_REF_PARAM, decls): + if (not Match(_RE_PATTERN_CONST_REF_PARAM, parameter) and + not Match(_RE_PATTERN_REF_STREAM_PARAM, parameter)): + error(filename, linenum, 'runtime/references', 2, + 'Is this a non-const reference? ' + 'If so, make const or use a pointer: ' + + ReplaceAll(' *<', '<', parameter)) + + +def CheckCasts(filename, clean_lines, linenum, error): + """Various cast related checks. - #if Search(r'\busing namespace\b', line): - # error(filename, linenum, 'build/namespaces', 5, - # 'Do not use namespace using-directives. ' - # 'Use using-declarations instead.') + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ + line = clean_lines.elided[linenum] - # Detect variable-length arrays. - match = Match(r'\s*(.+::)?(\w+) [a-z]\w*\[(.+)];', line) - if (match and match.group(2) != 'return' and match.group(2) != 'delete' and - match.group(3).find(']') == -1): - # Split the size using space and arithmetic operators as delimiters. - # If any of the resulting tokens are not compile time constants then - # report the error. - tokens = re.split(r'\s|\+|\-|\*|\/|<<|>>]', match.group(3)) - is_const = True - skip_next = False - for tok in tokens: - if skip_next: - skip_next = False - continue + # Check to see if they're using an conversion function cast. + # I just try to capture the most common basic types, though there are more. + # Parameterless conversion functions, such as bool(), are allowed as they are + # probably a member operator declaration or default constructor. + match = Search( + r'(\bnew\s+(?:const\s+)?|\S<\s*(?:const\s+)?)?\b' + r'(int|float|double|bool|char|int32|uint32|int64|uint64)' + r'(\([^)].*)', line) + expecting_function = ExpectingFunctionArgs(clean_lines, linenum) + if match and not expecting_function: + matched_type = match.group(2) + + # matched_new_or_template is used to silence two false positives: + # - New operators + # - Template arguments with function types + # + # For template arguments, we match on types immediately following + # an opening bracket without any spaces. This is a fast way to + # silence the common case where the function type is the first + # template argument. False negative with less-than comparison is + # avoided because those operators are usually followed by a space. + # + # function // bracket + no space = false positive + # value < double(42) // bracket + space = true positive + matched_new_or_template = match.group(1) - if Search(r'sizeof\(.+\)', tok): continue - if Search(r'arraysize\(\w+\)', tok): continue + # Avoid arrays by looking for brackets that come after the closing + # parenthesis. + if Match(r'\([^()]+\)\s*\[', match.group(3)): + return - tok = tok.lstrip('(') - tok = tok.rstrip(')') - if not tok: continue - if Match(r'\d+', tok): continue - if Match(r'0[xX][0-9a-fA-F]+', tok): continue - if Match(r'k[A-Z0-9]\w*', tok): continue - if Match(r'(.+::)?k[A-Z0-9]\w*', tok): continue - if Match(r'(.+::)?[A-Z][A-Z0-9_]*', tok): continue - # A catch all for tricky sizeof cases, including 'sizeof expression', - # 'sizeof(*type)', 'sizeof(const type)', 'sizeof(struct StructName)' - # requires skipping the next token because we split on ' ' and '*'. - if tok.startswith('sizeof'): - skip_next = True - continue - is_const = False - break - if not is_const: - error(filename, linenum, 'runtime/arrays', 1, - 'Do not use variable-length arrays. Use an appropriately named ' - "('k' followed by CamelCase) compile-time constant for the size.") + # Other things to ignore: + # - Function pointers + # - Casts to pointer types + # - Placement new + # - Alias declarations + matched_funcptr = match.group(3) + if (matched_new_or_template is None and + not (matched_funcptr and + (Match(r'\((?:[^() ]+::\s*\*\s*)?[^() ]+\)\s*\(', + matched_funcptr) or + matched_funcptr.startswith('(*)'))) and + not Match(r'\s*using\s+\S+\s*=\s*' + matched_type, line) and + not Search(r'new\(\S+\)\s*' + matched_type, line)): + error(filename, linenum, 'readability/casting', 4, + 'Using deprecated casting style. ' + 'Use static_cast<%s>(...) instead' % + matched_type) - # If DISALLOW_EVIL_CONSTRUCTORS, DISALLOW_COPY_AND_ASSIGN, or - # DISALLOW_IMPLICIT_CONSTRUCTORS is present, then it should be the last thing - # in the class declaration. - match = Match( - (r'\s*' - r'(DISALLOW_(EVIL_CONSTRUCTORS|COPY_AND_ASSIGN|IMPLICIT_CONSTRUCTORS))' - r'\(.*\);$'), - line) - if match and linenum + 1 < clean_lines.NumLines(): - next_line = clean_lines.elided[linenum + 1] - # We allow some, but not all, declarations of variables to be present - # in the statement that defines the class. The [\w\*,\s]* fragment of - # the regular expression below allows users to declare instances of - # the class or pointers to instances, but not less common types such - # as function pointers or arrays. It's a tradeoff between allowing - # reasonable code and avoiding trying to parse more C++ using regexps. - if not Search(r'^\s*}[\w\*,\s]*;', next_line): - error(filename, linenum, 'readability/constructors', 3, - match.group(1) + ' should be the last thing in the class') + if not expecting_function: + CheckCStyleCast(filename, clean_lines, linenum, 'static_cast', + r'\((int|float|double|bool|char|u?int(16|32|64))\)', error) - # Check for use of unnamed namespaces in header files. Registration - # macros are typically OK, so we allow use of "namespace {" on lines - # that end with backslashes. - if (file_extension == 'h' - and Search(r'\bnamespace\s*{', line) - and line[-1] != '\\'): - error(filename, linenum, 'build/namespaces', 4, - 'Do not use unnamed namespaces in header files. See ' - 'http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces' - ' for more information.') + # This doesn't catch all cases. Consider (const char * const)"hello". + # + # (char *) "foo" should always be a const_cast (reinterpret_cast won't + # compile). + if CheckCStyleCast(filename, clean_lines, linenum, 'const_cast', + r'\((char\s?\*+\s?)\)\s*"', error): + pass + else: + # Check pointer casts for other than string constants + CheckCStyleCast(filename, clean_lines, linenum, 'reinterpret_cast', + r'\((\w+\s?\*+\s?)\)', error) + + # In addition, we look for people taking the address of a cast. This + # is dangerous -- casts can assign to temporaries, so the pointer doesn't + # point where you think. + # + # Some non-identifier character is required before the '&' for the + # expression to be recognized as a cast. These are casts: + # expression = &static_cast(temporary()); + # function(&(int*)(temporary())); + # + # This is not a cast: + # reference_type&(int* function_param); + match = Search( + r'(?:[^\w]&\(([^)*][^)]*)\)[\w(])|' + r'(?:[^\w]&(static|dynamic|down|reinterpret)_cast\b)', line) + if match: + # Try a better error message when the & is bound to something + # dereferenced by the casted pointer, as opposed to the casted + # pointer itself. + parenthesis_error = False + match = Match(r'^(.*&(?:static|dynamic|down|reinterpret)_cast\b)<', line) + if match: + _, y1, x1 = CloseExpression(clean_lines, linenum, len(match.group(1))) + if x1 >= 0 and clean_lines.elided[y1][x1] == '(': + _, y2, x2 = CloseExpression(clean_lines, y1, x1) + if x2 >= 0: + extended_line = clean_lines.elided[y2][x2:] + if y2 < clean_lines.NumLines() - 1: + extended_line += clean_lines.elided[y2 + 1] + if Match(r'\s*(?:->|\[)', extended_line): + parenthesis_error = True + + if parenthesis_error: + error(filename, linenum, 'readability/casting', 4, + ('Are you taking an address of something dereferenced ' + 'from a cast? Wrapping the dereferenced expression in ' + 'parentheses will make the binding more obvious')) + else: + error(filename, linenum, 'runtime/casting', 4, + ('Are you taking an address of a cast? ' + 'This is dangerous: could be a temp var. ' + 'Take the address before doing the cast, rather than after')) -def CheckCStyleCast(filename, linenum, line, raw_line, cast_type, pattern, - error): +def CheckCStyleCast(filename, clean_lines, linenum, cast_type, pattern, error): """Checks for a C-style cast by looking for the pattern. - This also handles sizeof(type) warnings, due to similarity of content. - Args: filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. linenum: The number of the line to check. - line: The line of code to check. - raw_line: The raw line of code to check, with comments. cast_type: The string for the C++ cast to recommend. This is either reinterpret_cast, static_cast, or const_cast, depending. pattern: The regular expression used to find C-style casts. @@ -2792,56 +5843,65 @@ def CheckCStyleCast(filename, linenum, line, raw_line, cast_type, pattern, True if an error was emitted. False otherwise. """ + line = clean_lines.elided[linenum] match = Search(pattern, line) if not match: return False - # e.g., sizeof(int) - sizeof_match = Match(r'.*sizeof\s*$', line[0:match.start(1) - 1]) - if sizeof_match: - error(filename, linenum, 'runtime/sizeof', 1, - 'Using sizeof(type). Use sizeof(varname) instead if possible') - return True + # Exclude lines with keywords that tend to look like casts + context = line[0:match.start(1) - 1] + if Match(r'.*\b(?:sizeof|alignof|alignas|[_A-Z][_A-Z0-9]*)\s*$', context): + return False - typeid_match = Match(r'.*typeid\s*$',line[0:match.start(1) - 1]) - if typeid_match: - return True + # Try expanding current context to see if we one level of + # parentheses inside a macro. + if linenum > 0: + for i in xrange(linenum - 1, max(0, linenum - 5), -1): + context = clean_lines.elided[i] + context + if Match(r'.*\b[_A-Z][_A-Z0-9]*\s*\((?:\([^()]*\)|[^()])*$', context): + return False - remainder = line[match.end(0):] + # operator++(int) and operator--(int) + if context.endswith(' operator++') or context.endswith(' operator--'): + return False - # The close paren is for function pointers as arguments to a function. - # eg, void foo(void (*bar)(int)); - # The semicolon check is a more basic function check; also possibly a - # function pointer typedef. - # eg, void foo(int); or void foo(int) const; - # The equals check is for function pointer assignment. - # eg, void *(*foo)(int) = ... - # The > is for MockCallback<...> ... - # - # Right now, this will only catch cases where there's a single argument, and - # it's unnamed. It should probably be expanded to check for multiple - # arguments with some unnamed. - function_match = Match(r'\s*(\)|=|(const)?\s*(;|\{|throw\(\)|>))', remainder) - #print "Remainder[%s]" % remainder - if function_match: - if ('SIGNAL' not in raw_line and 'SLOT' not in raw_line and - (not function_match.group(3) or - function_match.group(3) == ';' or - ('<' not in raw_line and - '/*' not in raw_line))): - error(filename, linenum, 'readability/function', 3, - 'All parameters should be named in a function') - return True + # A single unnamed argument for a function tends to look like old style cast. + # If we see those, don't issue warnings for deprecated casts. + remainder = line[match.end(0):] + if Match(r'^\s*(?:;|const\b|throw\b|final\b|override\b|[=>{),]|->)', + remainder): + return False # At this point, all that should be left is actual casts. - if len(remainder) > 0 and 'public: ' not in raw_line and 'private:' not in raw_line and 'protected: ' not in raw_line and remainder != ' const' and '::' not in raw_line: - error(filename, linenum, 'readability/casting', 4, + error(filename, linenum, 'readability/casting', 4, 'Using C-style cast. Use %s<%s>(...) instead' % (cast_type, match.group(1))) return True +def ExpectingFunctionArgs(clean_lines, linenum): + """Checks whether where function type arguments are expected. + + Args: + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + + Returns: + True if the line at 'linenum' is inside something that expects arguments + of function types. + """ + line = clean_lines.elided[linenum] + return (Match(r'^\s*MOCK_(CONST_)?METHOD\d+(_T)?\(', line) or + (linenum >= 2 and + (Match(r'^\s*MOCK_(?:CONST_)?METHOD\d+(?:_T)?\((?:\S+,)?\s*$', + clean_lines.elided[linenum - 1]) or + Match(r'^\s*MOCK_(?:CONST_)?METHOD\d+(?:_T)?\(\s*$', + clean_lines.elided[linenum - 2]) or + Search(r'\bstd::m?function\s*\<\s*$', + clean_lines.elided[linenum - 1])))) + + _HEADERS_CONTAINING_TEMPLATES = ( ('', ('deque',)), ('', ('unary_function', 'binary_function', @@ -2863,12 +5923,16 @@ def CheckCStyleCast(filename, linenum, line, raw_line, cast_type, pattern, )), ('', ('numeric_limits',)), ('', ('list',)), - ('', ('map', 'multimap',)), - ('', ('allocator',)), + ('', ('multimap',)), + ('', ('allocator', 'make_shared', 'make_unique', 'shared_ptr', + 'unique_ptr', 'weak_ptr')), ('', ('queue', 'priority_queue',)), - ('', ('set', 'multiset',)), + ('', ('multiset',)), ('', ('stack',)), ('', ('char_traits', 'basic_string',)), + ('', ('tuple',)), + ('', ('unordered_map', 'unordered_multimap')), + ('', ('unordered_set', 'unordered_multiset')), ('', ('pair',)), ('', ('vector',)), @@ -2879,18 +5943,36 @@ def CheckCStyleCast(filename, linenum, line, raw_line, cast_type, pattern, ('', ('slist',)), ) -_RE_PATTERN_STRING = re.compile(r'\bstring\b') +_HEADERS_MAYBE_TEMPLATES = ( + ('', ('copy', 'max', 'min', 'min_element', 'sort', + 'transform', + )), + ('', ('forward', 'make_pair', 'move', 'swap')), + ) -_re_pattern_algorithm_header = [] -for _template in ('copy', 'max', 'min', 'min_element', 'sort', 'swap', - 'transform'): - # Match max(..., ...), max(..., ...), but not foo->max, foo.max or - # type::max(). - _re_pattern_algorithm_header.append( - (re.compile(r'[^>.]\b' + _template + r'(<.*?>)?\([^\)]'), - _template, - '')) +_RE_PATTERN_STRING = re.compile(r'\bstring\b') +_re_pattern_headers_maybe_templates = [] +for _header, _templates in _HEADERS_MAYBE_TEMPLATES: + for _template in _templates: + # Match max(..., ...), max(..., ...), but not foo->max, foo.max or + # 'type::max()'. + _re_pattern_headers_maybe_templates.append( + (re.compile(r'[^>.]\b' + _template + r'(<.*?>)?\([^\)]'), + _template, + _header)) +# Match set, but not foo->set, foo.set +_re_pattern_headers_maybe_templates.append( + (re.compile(r'[^>.]\bset\s*\<'), + 'set<>', + '')) +# Match 'map var' and 'std::map(...)', but not 'map(...)'' +_re_pattern_headers_maybe_templates.append( + (re.compile(r'(std\b::\bmap\s*\<)|(^(std\b::\b)map\b\(\s*\<)'), + 'map<>', + '')) + +# Other scripts may reach in and modify this pattern. _re_pattern_templates = [] for _header, _templates in _HEADERS_CONTAINING_TEMPLATES: for _template in _templates: @@ -2921,7 +6003,7 @@ def FilesBelongToSameModule(filename_cc, filename_h): some false positives. This should be sufficiently rare in practice. Args: - filename_cc: is the path for the .cc file + filename_cc: is the path for the source (e.g. .cc) file filename_h: is the path for the header path Returns: @@ -2929,20 +6011,23 @@ def FilesBelongToSameModule(filename_cc, filename_h): bool: True if filename_cc and filename_h belong to the same module. string: the additional prefix needed to open the header file. """ + fileinfo_cc = FileInfo(filename_cc) + if not fileinfo_cc.Extension().lstrip('.') in GetNonHeaderExtensions(): + return (False, '') - if not filename_cc.endswith('.cc'): + fileinfo_h = FileInfo(filename_h) + if not IsHeaderExtension(fileinfo_h.Extension().lstrip('.')): return (False, '') - filename_cc = filename_cc[:-len('.cc')] - if filename_cc.endswith('_unittest'): - filename_cc = filename_cc[:-len('_unittest')] - elif filename_cc.endswith('_test'): - filename_cc = filename_cc[:-len('_test')] + + filename_cc = filename_cc[:-(len(fileinfo_cc.Extension()))] + matched_test_suffix = Search(_TEST_FILE_SUFFIX, fileinfo_cc.BaseName()) + if matched_test_suffix: + filename_cc = filename_cc[:-len(matched_test_suffix.group(1))] + filename_cc = filename_cc.replace('/public/', '/') filename_cc = filename_cc.replace('/internal/', '/') - if not filename_h.endswith('.h'): - return (False, '') - filename_h = filename_h[:-len('.h')] + filename_h = filename_h[:-(len(fileinfo_h.Extension()))] if filename_h.endswith('-inl'): filename_h = filename_h[:-len('-inl')] filename_h = filename_h.replace('/public/', '/') @@ -2955,33 +6040,32 @@ def FilesBelongToSameModule(filename_cc, filename_h): return files_belong_to_same_module, common_path -def UpdateIncludeState(filename, include_state, io=codecs): - """Fill up the include_state with new includes found from the file. +def UpdateIncludeState(filename, include_dict, io=codecs): + """Fill up the include_dict with new includes found from the file. Args: filename: the name of the header to read. - include_state: an _IncludeState instance in which the headers are inserted. + include_dict: a dictionary in which the headers are inserted. io: The io factory to use to read the file. Provided for testability. Returns: - True if a header was succesfully added. False otherwise. + True if a header was successfully added. False otherwise. """ headerfile = None try: - headerfile = io.open(filename, 'r', 'utf8', 'replace') + with io.open(filename, 'r', 'utf8', 'replace') as headerfile: + linenum = 0 + for line in headerfile: + linenum += 1 + clean_line = CleanseComments(line) + match = _RE_PATTERN_INCLUDE.search(clean_line) + if match: + include = match.group(2) + include_dict.setdefault(include, linenum) + return True except IOError: return False - linenum = 0 - for line in headerfile: - linenum += 1 - clean_line = CleanseComments(line) - match = _RE_PATTERN_INCLUDE.search(clean_line) - if match: - include = match.group(2) - # The value formatting is cute, but not really used right now. - # What matters here is that the key is in include_state. - include_state.setdefault(include, '%s:%d' % (filename, linenum)) - return True + def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error, @@ -3019,7 +6103,7 @@ def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error, if prefix.endswith('std::') or not prefix.endswith('::'): required[''] = (linenum, 'string') - for pattern, template, header in _re_pattern_algorithm_header: + for pattern, template, header in _re_pattern_headers_maybe_templates: if pattern.search(line): required[header] = (linenum, template) @@ -3028,19 +6112,25 @@ def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error, continue for pattern, template, header in _re_pattern_templates: - if pattern.search(line): - required[header] = (linenum, template) + matched = pattern.search(line) + if matched: + # Don't warn about IWYU in non-STL namespaces: + # (We check only the first match per line; good enough.) + prefix = line[:matched.start()] + if prefix.endswith('std::') or not prefix.endswith('::'): + required[header] = (linenum, template) # The policy is that if you #include something in foo.h you don't need to # include it again in foo.cc. Here, we will look at possible includes. - # Let's copy the include_state so it is only messed up within this function. - include_state = include_state.copy() + # Let's flatten the include_state include_list and copy it into a dictionary. + include_dict = dict([item for sublist in include_state.include_list + for item in sublist]) - # Did we find the header for this file (if any) and succesfully load it? + # Did we find the header for this file (if any) and successfully load it? header_found = False # Use the absolute path so that matching works properly. - abs_filename = os.path.abspath(filename) + abs_filename = FileInfo(filename).FullName() # For Emacs's flymake. # If cpplint is invoked from Emacs's flymake, a temporary file is generated @@ -3051,13 +6141,13 @@ def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error, # instead of 'foo_flymake.h' abs_filename = re.sub(r'_flymake\.cc$', '.cc', abs_filename) - # include_state is modified during iteration, so we iterate over a copy of + # include_dict is modified during iteration, so we iterate over a copy of # the keys. - header_keys = include_state.keys() + header_keys = list(include_dict.keys()) for header in header_keys: (same_module, common_path) = FilesBelongToSameModule(abs_filename, header) fullpath = common_path + header - if same_module and UpdateIncludeState(fullpath, include_state, io): + if same_module and UpdateIncludeState(fullpath, include_dict, io): header_found = True # If we can't find the header file for a .cc, assume it's because we don't @@ -3065,13 +6155,15 @@ def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error, # didn't include it in the .h file. # TODO(unknown): Do a better job of finding .h files so we are confident that # not having the .h file means there isn't one. - if filename.endswith('.cc') and not header_found: - return + if not header_found: + for extension in GetNonHeaderExtensions(): + if filename.endswith('.' + extension): + return # All the lines have been processed, report the errors found. - for required_header_unstripped in required: + for required_header_unstripped in sorted(required, key=required.__getitem__): template = required[required_header_unstripped][1] - if required_header_unstripped.strip('<>"') not in include_state: + if required_header_unstripped.strip('<>"') not in include_dict: error(filename, required[required_header_unstripped][0], 'build/include_what_you_use', 4, 'Add #include ' + required_header_unstripped + ' for ' + template) @@ -3083,7 +6175,7 @@ def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error, def CheckMakePairUsesDeduction(filename, clean_lines, linenum, error): """Check that make_pair's template arguments are deduced. - G++ 4.6 in C++0x mode fails badly if make_pair's template arguments are + G++ 4.6 in C++11 mode fails badly if make_pair's template arguments are specified explicitly, and such use isn't intended in any case. Args: @@ -3092,19 +6184,175 @@ def CheckMakePairUsesDeduction(filename, clean_lines, linenum, error): linenum: The number of the line to check. error: The function to call with any errors found. """ - raw = clean_lines.raw_lines - line = raw[linenum] + line = clean_lines.elided[linenum] match = _RE_PATTERN_EXPLICIT_MAKEPAIR.search(line) if match: error(filename, linenum, 'build/explicit_make_pair', 4, # 4 = high confidence - 'Omit template arguments from make_pair OR use pair directly OR' - ' if appropriate, construct a pair directly') + 'For C++11-compatibility, omit template arguments from make_pair' + ' OR use pair directly OR if appropriate, construct a pair directly') + + +def CheckRedundantVirtual(filename, clean_lines, linenum, error): + """Check if line contains a redundant "virtual" function-specifier. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ + # Look for "virtual" on current line. + line = clean_lines.elided[linenum] + virtual = Match(r'^(.*)(\bvirtual\b)(.*)$', line) + if not virtual: return + + # Ignore "virtual" keywords that are near access-specifiers. These + # are only used in class base-specifier and do not apply to member + # functions. + if (Search(r'\b(public|protected|private)\s+$', virtual.group(1)) or + Match(r'^\s+(public|protected|private)\b', virtual.group(3))): + return + + # Ignore the "virtual" keyword from virtual base classes. Usually + # there is a column on the same line in these cases (virtual base + # classes are rare in google3 because multiple inheritance is rare). + if Match(r'^.*[^:]:[^:].*$', line): return + + # Look for the next opening parenthesis. This is the start of the + # parameter list (possibly on the next line shortly after virtual). + # TODO(unknown): doesn't work if there are virtual functions with + # decltype() or other things that use parentheses, but csearch suggests + # that this is rare. + end_col = -1 + end_line = -1 + start_col = len(virtual.group(2)) + for start_line in xrange(linenum, min(linenum + 3, clean_lines.NumLines())): + line = clean_lines.elided[start_line][start_col:] + parameter_list = Match(r'^([^(]*)\(', line) + if parameter_list: + # Match parentheses to find the end of the parameter list + (_, end_line, end_col) = CloseExpression( + clean_lines, start_line, start_col + len(parameter_list.group(1))) + break + start_col = 0 + + if end_col < 0: + return # Couldn't find end of parameter list, give up + + # Look for "override" or "final" after the parameter list + # (possibly on the next few lines). + for i in xrange(end_line, min(end_line + 3, clean_lines.NumLines())): + line = clean_lines.elided[i][end_col:] + match = Search(r'\b(override|final)\b', line) + if match: + error(filename, linenum, 'readability/inheritance', 4, + ('"virtual" is redundant since function is ' + 'already declared as "%s"' % match.group(1))) + + # Set end_col to check whole lines after we are done with the + # first line. + end_col = 0 + if Search(r'[^\w]\s*$', line): + break + + +def CheckRedundantOverrideOrFinal(filename, clean_lines, linenum, error): + """Check if line contains a redundant "override" or "final" virt-specifier. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ + # Look for closing parenthesis nearby. We need one to confirm where + # the declarator ends and where the virt-specifier starts to avoid + # false positives. + line = clean_lines.elided[linenum] + declarator_end = line.rfind(')') + if declarator_end >= 0: + fragment = line[declarator_end:] + else: + if linenum > 1 and clean_lines.elided[linenum - 1].rfind(')') >= 0: + fragment = line + else: + return + + # Check that at most one of "override" or "final" is present, not both + if Search(r'\boverride\b', fragment) and Search(r'\bfinal\b', fragment): + error(filename, linenum, 'readability/inheritance', 4, + ('"override" is redundant since function is ' + 'already declared as "final"')) + + + + +# Returns true if we are at a new block, and it is directly +# inside of a namespace. +def IsBlockInNameSpace(nesting_state, is_forward_declaration): + """Checks that the new block is directly in a namespace. + + Args: + nesting_state: The _NestingState object that contains info about our state. + is_forward_declaration: If the class is a forward declared class. + Returns: + Whether or not the new block is directly in a namespace. + """ + if is_forward_declaration: + return len(nesting_state.stack) >= 1 and ( + isinstance(nesting_state.stack[-1], _NamespaceInfo)) + + + return (len(nesting_state.stack) > 1 and + nesting_state.stack[-1].check_namespace_indentation and + isinstance(nesting_state.stack[-2], _NamespaceInfo)) + + +def ShouldCheckNamespaceIndentation(nesting_state, is_namespace_indent_item, + raw_lines_no_comments, linenum): + """This method determines if we should apply our namespace indentation check. + + Args: + nesting_state: The current nesting state. + is_namespace_indent_item: If we just put a new class on the stack, True. + If the top of the stack is not a class, or we did not recently + add the class, False. + raw_lines_no_comments: The lines without the comments. + linenum: The current line number we are processing. + Returns: + True if we should apply our namespace indentation check. Currently, it + only works for classes and namespaces inside of a namespace. + """ + + is_forward_declaration = IsForwardClassDeclaration(raw_lines_no_comments, + linenum) + + if not (is_namespace_indent_item or is_forward_declaration): + return False + + # If we are in a macro, we do not want to check the namespace indentation. + if IsMacroDefinition(raw_lines_no_comments, linenum): + return False + + return IsBlockInNameSpace(nesting_state, is_forward_declaration) -def ProcessLine(filename, file_extension, - clean_lines, line, include_state, function_state, - class_state, error, extra_check_functions=[]): + +# Call this method if the line is directly inside of a namespace. +# If the line above is blank (excluding comments) or the start of +# an inner namespace, it cannot be indented. +def CheckItemIndentationInNamespace(filename, raw_lines_no_comments, linenum, + error): + line = raw_lines_no_comments[linenum] + if Match(r'^\s+', line): + error(filename, linenum, 'runtime/indentation_namespace', 4, + 'Do not indent within a namespace') + + +def ProcessLine(filename, file_extension, clean_lines, line, + include_state, function_state, nesting_state, error, + extra_check_functions=None): """Processes a single line in the file. Args: @@ -3115,8 +6363,8 @@ def ProcessLine(filename, file_extension, line: Number of line being processed. include_state: An _IncludeState instance in which the headers are inserted. function_state: A _FunctionState instance which counts function lines, etc. - class_state: A _ClassState instance which maintains information about - the current stack of nested class declarations being parsed. + nesting_state: A NestingState instance which maintains information about + the current stack of nested blocks being parsed. error: A callable to which errors are reported, which takes 4 arguments: filename, line number, error level, and message extra_check_functions: An array of additional check functions that will be @@ -3125,21 +6373,101 @@ def ProcessLine(filename, file_extension, """ raw_lines = clean_lines.raw_lines ParseNolintSuppressions(filename, raw_lines[line], line, error) + nesting_state.Update(filename, clean_lines, line, error) + CheckForNamespaceIndentation(filename, nesting_state, clean_lines, line, + error) + if nesting_state.InAsmBlock(): return CheckForFunctionLengths(filename, clean_lines, line, function_state, error) CheckForMultilineCommentsAndStrings(filename, clean_lines, line, error) - CheckStyle(filename, clean_lines, line, file_extension, class_state, error) + CheckStyle(filename, clean_lines, line, file_extension, nesting_state, error) CheckLanguage(filename, clean_lines, line, file_extension, include_state, - error) + nesting_state, error) + CheckForNonConstReference(filename, clean_lines, line, nesting_state, error) CheckForNonStandardConstructs(filename, clean_lines, line, - class_state, error) + nesting_state, error) + CheckVlogArguments(filename, clean_lines, line, error) CheckPosixThreading(filename, clean_lines, line, error) CheckInvalidIncrement(filename, clean_lines, line, error) CheckMakePairUsesDeduction(filename, clean_lines, line, error) - for check_fn in extra_check_functions: - check_fn(filename, clean_lines, line, error) + CheckRedundantVirtual(filename, clean_lines, line, error) + CheckRedundantOverrideOrFinal(filename, clean_lines, line, error) + if extra_check_functions: + for check_fn in extra_check_functions: + check_fn(filename, clean_lines, line, error) + +def FlagCxx11Features(filename, clean_lines, linenum, error): + """Flag those c++11 features that we only allow in certain places. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ + line = clean_lines.elided[linenum] + + include = Match(r'\s*#\s*include\s+[<"]([^<"]+)[">]', line) + + # Flag unapproved C++ TR1 headers. + if include and include.group(1).startswith('tr1/'): + error(filename, linenum, 'build/c++tr1', 5, + ('C++ TR1 headers such as <%s> are unapproved.') % include.group(1)) + + # Flag unapproved C++11 headers. + if include and include.group(1) in ('cfenv', + 'condition_variable', + 'fenv.h', + 'future', + 'mutex', + 'thread', + 'chrono', + 'ratio', + 'regex', + 'system_error', + ): + error(filename, linenum, 'build/c++11', 5, + ('<%s> is an unapproved C++11 header.') % include.group(1)) + + # The only place where we need to worry about C++11 keywords and library + # features in preprocessor directives is in macro definitions. + if Match(r'\s*#', line) and not Match(r'\s*#\s*define\b', line): return + + # These are classes and free functions. The classes are always + # mentioned as std::*, but we only catch the free functions if + # they're not found by ADL. They're alphabetical by header. + for top_name in ( + # type_traits + 'alignment_of', + 'aligned_union', + ): + if Search(r'\bstd::%s\b' % top_name, line): + error(filename, linenum, 'build/c++11', 5, + ('std::%s is an unapproved C++11 class or function. Send c-style ' + 'an example of where it would make your code more readable, and ' + 'they may let you use it.') % top_name) + + +def FlagCxx14Features(filename, clean_lines, linenum, error): + """Flag those C++14 features that we restrict. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ + line = clean_lines.elided[linenum] + + include = Match(r'\s*#\s*include\s+[<"]([^<"]+)[">]', line) + + # Flag unapproved C++14 headers. + if include and include.group(1) in ('scoped_allocator', 'shared_mutex'): + error(filename, linenum, 'build/c++14', 5, + ('<%s> is an unapproved C++14 header.') % include.group(1)) + def ProcessFileData(filename, file_extension, lines, error, - extra_check_functions=[]): + extra_check_functions=None): """Performs lint checks and reports any errors to the given error function. Args: @@ -3158,32 +6486,127 @@ def ProcessFileData(filename, file_extension, lines, error, include_state = _IncludeState() function_state = _FunctionState() - class_state = _ClassState() + nesting_state = NestingState() ResetNolintSuppressions() CheckForCopyright(filename, lines, error) - - if file_extension == 'h': - CheckForHeaderGuard(filename, lines, error) - + ProcessGlobalSuppresions(lines) RemoveMultiLineComments(filename, lines, error) clean_lines = CleansedLines(lines) + + if IsHeaderExtension(file_extension): + CheckForHeaderGuard(filename, clean_lines, error) + for line in xrange(clean_lines.NumLines()): ProcessLine(filename, file_extension, clean_lines, line, - include_state, function_state, class_state, error, + include_state, function_state, nesting_state, error, extra_check_functions) - class_state.CheckFinished(filename, error) + FlagCxx11Features(filename, clean_lines, line, error) + nesting_state.CheckCompletedBlocks(filename, error) CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error) + # Check that the .cc file has included its header if it exists. + if _IsSourceExtension(file_extension): + CheckHeaderFileIncluded(filename, include_state, error) + # We check here rather than inside ProcessLine so that we see raw # lines rather than "cleaned" lines. - CheckForUnicodeReplacementCharacters(filename, lines, error) + CheckForBadCharacters(filename, lines, error) CheckForNewlineAtEOF(filename, lines, error) -def ProcessFile(filename, vlevel, extra_check_functions=[]): +def ProcessConfigOverrides(filename): + """ Loads the configuration files and processes the config overrides. + + Args: + filename: The name of the file being processed by the linter. + + Returns: + False if the current |filename| should not be processed further. + """ + + abs_filename = os.path.abspath(filename) + cfg_filters = [] + keep_looking = True + while keep_looking: + abs_path, base_name = os.path.split(abs_filename) + if not base_name: + break # Reached the root directory. + + cfg_file = os.path.join(abs_path, "CPPLINT.cfg") + abs_filename = abs_path + if not os.path.isfile(cfg_file): + continue + + try: + with open(cfg_file) as file_handle: + for line in file_handle: + line, _, _ = line.partition('#') # Remove comments. + if not line.strip(): + continue + + name, _, val = line.partition('=') + name = name.strip() + val = val.strip() + if name == 'set noparent': + keep_looking = False + elif name == 'filter': + cfg_filters.append(val) + elif name == 'exclude_files': + # When matching exclude_files pattern, use the base_name of + # the current file name or the directory name we are processing. + # For example, if we are checking for lint errors in /foo/bar/baz.cc + # and we found the .cfg file at /foo/CPPLINT.cfg, then the config + # file's "exclude_files" filter is meant to be checked against "bar" + # and not "baz" nor "bar/baz.cc". + if base_name: + pattern = re.compile(val) + if pattern.match(base_name): + if _cpplint_state.quiet: + # Suppress "Ignoring file" warning when using --quiet. + return False + _cpplint_state.PrintInfo('Ignoring "%s": file excluded by "%s". ' + 'File path component "%s" matches ' + 'pattern "%s"\n' % + (filename, cfg_file, base_name, val)) + return False + elif name == 'linelength': + global _line_length + try: + _line_length = int(val) + except ValueError: + _cpplint_state.PrintError('Line length must be numeric.') + elif name == 'extensions': + ProcessExtensionsOption(val) + elif name == 'root': + global _root + # root directories are specified relative to CPPLINT.cfg dir. + _root = os.path.join(os.path.dirname(cfg_file), val) + elif name == 'headers': + ProcessHppHeadersOption(val) + elif name == 'includeorder': + ProcessIncludeOrderOption(val) + else: + _cpplint_state.PrintError( + 'Invalid configuration option (%s) in file %s\n' % + (name, cfg_file)) + + except IOError: + _cpplint_state.PrintError( + "Skipping config file '%s': Can't open for reading\n" % cfg_file) + keep_looking = False + + # Apply all the accumulated filters in reverse order (top-level directory + # config options having the least priority). + for cfg_filter in reversed(cfg_filters): + _AddFilters(cfg_filter) + + return True + + +def ProcessFile(filename, vlevel, extra_check_functions=None): """Does google-lint on a single file. Args: @@ -3198,7 +6621,15 @@ def ProcessFile(filename, vlevel, extra_check_functions=[]): """ _SetVerboseLevel(vlevel) + _BackupFilters() + old_errors = _cpplint_state.error_count + + if not ProcessConfigOverrides(filename): + _RestoreFilters() + return + lf_lines = [] + crlf_lines = [] try: # Support the UNIX convention of using "-" for stdin. Note that # we are not opening the file with universal newline support @@ -3206,28 +6637,29 @@ def ProcessFile(filename, vlevel, extra_check_functions=[]): # contain trailing '\r' characters if we are reading a file that # has CRLF endings. # If after the split a trailing '\r' is present, it is removed - # below. If it is not expected to be present (i.e. os.linesep != - # '\r\n' as in Windows), a warning is issued below if this file - # is processed. - + # below. if filename == '-': lines = codecs.StreamReaderWriter(sys.stdin, codecs.getreader('utf8'), codecs.getwriter('utf8'), 'replace').read().split('\n') else: - lines = codecs.open(filename, 'r', 'utf8', 'replace').read().split('\n') + with codecs.open(filename, 'r', 'utf8', 'replace') as target_file: + lines = target_file.read().split('\n') - carriage_return_found = False # Remove trailing '\r'. - for linenum in range(len(lines)): + # The -1 accounts for the extra trailing blank line we get from split() + for linenum in range(len(lines) - 1): if lines[linenum].endswith('\r'): lines[linenum] = lines[linenum].rstrip('\r') - carriage_return_found = True + crlf_lines.append(linenum + 1) + else: + lf_lines.append(linenum + 1) except IOError: - #sys.stderr.write( - # "Skipping input '%s': Can't open for reading\n" % filename) + _cpplint_state.PrintError( + "Skipping input '%s': Can't open for reading\n" % filename) + _RestoreFilters() return # Note, if no dot is found, this will give the entire filename as the ext. @@ -3235,23 +6667,40 @@ def ProcessFile(filename, vlevel, extra_check_functions=[]): # When reading from stdin, the extension is unknown, so no cpplint tests # should rely on the extension. - if (filename != '-' and file_extension != 'cc' and file_extension != 'h' - and file_extension != 'cpp' and file_extension != 'hh'): - if (filename != "CMakeLists.txt" and file_extension != "in" - and file_extension != "proto" and file_extension != "sdf"): - pass - #sys.stderr.write('Ignoring %s; not a .cc or .h file\n' % filename) + if filename != '-' and file_extension not in GetAllExtensions(): + pass + # Ignition: never print this + # _cpplint_state.PrintError('Ignoring %s; not a valid file name ' + # '(%s)\n' % (filename, ', '.join(GetAllExtensions()))) else: ProcessFileData(filename, file_extension, lines, Error, extra_check_functions) - if carriage_return_found and os.linesep != '\r\n': - # Use 0 for linenum since outputting only one error for potentially - # several lines. - Error(filename, 0, 'whitespace/newline', 1, - 'One or more unexpected \\r (^M) found;' - 'better to use only a \\n') - #sys.stderr.write('Done processing %s\n' % filename) + # If end-of-line sequences are a mix of LF and CR-LF, issue + # warnings on the lines with CR. + # + # Don't issue any warnings if all lines are uniformly LF or CR-LF, + # since critique can handle these just fine, and the style guide + # doesn't dictate a particular end of line sequence. + # + # We can't depend on os.linesep to determine what the desired + # end-of-line sequence should be, since that will return the + # server-side end-of-line sequence. + if lf_lines and crlf_lines: + # Warn on every line with CR. An alternative approach might be to + # check whether the file is mostly CRLF or just LF, and warn on the + # minority, we bias toward LF here since most tools prefer LF. + for linenum in crlf_lines: + Error(filename, linenum, 'whitespace/newline', 1, + 'Unexpected \\r (^M) found; better to use only \\n') + + # Suppress printing anything if --quiet was passed unless the error + # count has increased after processing this file. + if not _cpplint_state.quiet or old_errors != _cpplint_state.error_count: + pass + # Ignition: never print "Done Processing" + # _cpplint_state.PrintInfo('Done processing %s\n' % filename) + _RestoreFilters() def PrintUsage(message): @@ -3260,12 +6709,21 @@ def PrintUsage(message): Args: message: The optional error message. """ - sys.stderr.write(_USAGE) + sys.stderr.write(_USAGE % (list(GetAllExtensions()), + ','.join(list(GetAllExtensions())), + GetHeaderExtensions(), + ','.join(GetHeaderExtensions()))) + if message: sys.exit('\nFATAL ERROR: ' + message) else: - sys.exit(1) + sys.exit(0) +def PrintVersion(): + sys.stdout.write('Cpplint fork (https://github.com/cpplint/cpplint)\n') + sys.stdout.write('cpplint ' + __VERSION__ + '\n') + sys.stdout.write('Python ' + sys.version + '\n') + sys.exit(0) def PrintCategories(): """Prints a list of all the error-categories used by error messages. @@ -3289,24 +6747,42 @@ def ParseArguments(args): """ try: (opts, filenames) = getopt.getopt(args, '', ['help', 'output=', 'verbose=', + 'v=', + 'version', 'counting=', - 'filter=']) + 'filter=', + 'root=', + 'repository=', + 'linelength=', + 'extensions=', + 'exclude=', + 'recursive', + 'headers=', + 'includeorder=', + 'quiet']) except getopt.GetoptError: PrintUsage('Invalid arguments.') verbosity = _VerboseLevel() output_format = _OutputFormat() filters = '' + quiet = _Quiet() counting_style = '' + recursive = False for (opt, val) in opts: if opt == '--help': PrintUsage(None) + if opt == '--version': + PrintVersion() elif opt == '--output': - if not val in ('emacs', 'vs7'): - PrintUsage('The only allowed output formats are emacs and vs7.') + if val not in ('emacs', 'vs7', 'eclipse', 'junit', 'sed', 'gsed'): + PrintUsage('The only allowed output formats are emacs, vs7, eclipse ' + 'sed, gsed and junit.') output_format = val - elif opt == '--verbose': + elif opt == '--quiet': + quiet = True + elif opt == '--verbose' or opt == '--v': verbosity = int(val) elif opt == '--filter': filters = val @@ -3316,33 +6792,129 @@ def ParseArguments(args): if val not in ('total', 'toplevel', 'detailed'): PrintUsage('Valid counting options are total, toplevel, and detailed') counting_style = val + elif opt == '--root': + global _root + _root = val + elif opt == '--repository': + global _repository + _repository = val + elif opt == '--linelength': + global _line_length + try: + _line_length = int(val) + except ValueError: + PrintUsage('Line length must be digits.') + elif opt == '--exclude': + global _excludes + if not _excludes: + _excludes = set() + _excludes.update(glob.glob(val)) + elif opt == '--extensions': + ProcessExtensionsOption(val) + elif opt == '--headers': + ProcessHppHeadersOption(val) + elif opt == '--recursive': + recursive = True + elif opt == '--includeorder': + ProcessIncludeOrderOption(val) if not filenames: PrintUsage('No files were specified.') + if recursive: + filenames = _ExpandDirectories(filenames) + + if _excludes: + filenames = _FilterExcludedFiles(filenames) + _SetOutputFormat(output_format) + _SetQuiet(quiet) _SetVerboseLevel(verbosity) _SetFilters(filters) _SetCountingStyle(counting_style) + filenames.sort() return filenames +def _ExpandDirectories(filenames): + """Searches a list of filenames and replaces directories in the list with + all files descending from those directories. Files with extensions not in + the valid extensions list are excluded. + + Args: + filenames: A list of files or directories + + Returns: + A list of all files that are members of filenames or descended from a + directory in filenames + """ + expanded = set() + for filename in filenames: + if not os.path.isdir(filename): + expanded.add(filename) + continue + + for root, _, files in os.walk(filename): + for loopfile in files: + fullname = os.path.join(root, loopfile) + if fullname.startswith('.' + os.path.sep): + fullname = fullname[len('.' + os.path.sep):] + expanded.add(fullname) + + filtered = [] + for filename in expanded: + if os.path.splitext(filename)[1][1:] in GetAllExtensions(): + filtered.append(filename) + return filtered + +def _FilterExcludedFiles(fnames): + """Filters out files listed in the --exclude command line switch. File paths + in the switch are evaluated relative to the current working directory + """ + exclude_paths = [os.path.abspath(f) for f in _excludes] + # because globbing does not work recursively, exclude all subpath of all excluded entries + return [f for f in fnames + if not any(e for e in exclude_paths + if _IsParentOrSame(e, os.path.abspath(f)))] + +def _IsParentOrSame(parent, child): + """Return true if child is subdirectory of parent. + Assumes both paths are absolute and don't contain symlinks. + """ + parent = os.path.normpath(parent) + child = os.path.normpath(child) + if parent == child: + return True + + prefix = os.path.commonprefix([parent, child]) + if prefix != parent: + return False + # Note: os.path.commonprefix operates on character basis, so + # take extra care of situations like '/foo/ba' and '/foo/bar/baz' + child_suffix = child[len(prefix):] + child_suffix = child_suffix.lstrip(os.sep) + return child == os.path.join(prefix, child_suffix) def main(): filenames = ParseArguments(sys.argv[1:]) + backup_err = sys.stderr + try: + # Change stderr to write with replacement characters so we don't die + # if we try to print something containing non-ASCII characters. + sys.stderr = codecs.StreamReader(sys.stderr, 'replace') - # Change stderr to write with replacement characters so we don't die - # if we try to print something containing non-ASCII characters. - sys.stderr = codecs.StreamReaderWriter(sys.stderr, - codecs.getreader('utf8'), - codecs.getwriter('utf8'), - 'replace') - - _cpplint_state.ResetErrorCounts() - for filename in filenames: - if filename != "./gazebo/rendering/cegui.h": + _cpplint_state.ResetErrorCounts() + for filename in filenames: ProcessFile(filename, _cpplint_state.verbose_level) - _cpplint_state.PrintErrorCounts() + # If --quiet is passed, suppress printing error count unless there are errors. + if not _cpplint_state.quiet or _cpplint_state.error_count > 0: + _cpplint_state.PrintErrorCounts() + + if _cpplint_state.output_format == 'junit': + sys.stderr.write(_cpplint_state.FormatJUnitXML()) + + finally: + sys.stderr = backup_err sys.exit(_cpplint_state.error_count > 0)