Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix more C++ lints in cpp #3275

Merged
merged 4 commits into from
Dec 17, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .clang-tidy
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ Checks:
'-*,
clang-analyzer-*,
-clang-diagnostic-shadow-uncaptured-local,
-clang-analyzer-optin.core.EnumCastOutOfRange,
cert-*,
modernize-*,
-modernize-avoid-c-arrays,
Expand Down
8 changes: 4 additions & 4 deletions cpp/include/DataStorm/InternalT.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,13 @@ namespace DataStormI
template<typename T> class has_communicator_parameter
{
template<typename TT, typename SS>
static auto testE(int)
static auto testE(int) noexcept
-> decltype(TT::encode(std::declval<Ice::CommunicatorPtr&>(), std::declval<SS&>()), std::true_type());

template<typename, typename> static auto testE(...) -> std::false_type;

template<typename TT, typename SS>
static auto testD(int)
static auto testD(int) noexcept
-> decltype(TT::decode(std::declval<Ice::CommunicatorPtr&>(), Ice::ByteSeq()), std::true_type());

template<typename, typename> static auto testD(...) -> std::false_type;
Expand Down Expand Up @@ -79,9 +79,9 @@ namespace DataStormI
template<typename T> class is_streamable
{
template<typename TT, typename SS>
static auto test(int) -> decltype(std::declval<SS&>() << std::declval<TT>(), std::true_type());
static auto test(int) noexcept -> decltype(std::declval<SS&>() << std::declval<TT>(), std::true_type());

template<typename, typename> static auto test(...) -> std::false_type;
template<typename, typename> static auto test(...) noexcept -> std::false_type;

public:
static const bool value = decltype(test<T, std::ostream>(0))::value;
Expand Down
4 changes: 2 additions & 2 deletions cpp/include/Ice/LoggerUtil.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ namespace Ice

template<typename T> struct IsException
{
static char testex(Ice::Exception*);
static long testex(...);
static char testex(Ice::Exception*) noexcept;
static long testex(...) noexcept;

static const bool value = sizeof(testex(static_cast<T*>(nullptr))) == sizeof(char);
};
Expand Down
4 changes: 2 additions & 2 deletions cpp/include/Ice/Proxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -259,8 +259,8 @@ namespace Ice

// The copy constructor and assignment operators are to keep GCC happy.
Proxy(const Proxy&) noexcept = default;
Proxy& operator=(const Proxy&) noexcept { return *this; }
Proxy& operator=(Proxy&&) noexcept { return *this; }
Proxy& operator=(const Proxy&) noexcept { return *this; } // NOLINT
Copy link
Member

Choose a reason for hiding this comment

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

Can we include the lint name like we're doing in other places.

Copy link
Member Author

Choose a reason for hiding this comment

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

These are dummy functions to keep GCC happy. Here, clang-tidy is complaining about not testing for self-assignment, which is irrelevant.

Proxy& operator=(Proxy&&) noexcept { return *this; } // NOLINT

private:
Prx fromReference(IceInternal::ReferencePtr&& ref) const
Expand Down
8 changes: 4 additions & 4 deletions cpp/include/Ice/StreamableTraits.h
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,9 @@ namespace Ice
*/
template<typename T> struct IsContainer
{
template<typename C> static char test(typename C::iterator*);
template<typename C> static char test(typename C::iterator*) noexcept;

template<typename C> static long test(...);
template<typename C> static long test(...) noexcept;

static const bool value = sizeof(test<T>(0)) == sizeof(char);
};
Expand All @@ -95,9 +95,9 @@ namespace Ice
*/
template<typename T> struct IsMap
{
template<typename C> static char test(typename C::mapped_type*);
template<typename C> static char test(typename C::mapped_type*) noexcept;

template<typename C> static long test(...);
template<typename C> static long test(...) noexcept;

static const bool value = IsContainer<T>::value && sizeof(test<T>(0)) == sizeof(char);
};
Expand Down
4 changes: 2 additions & 2 deletions cpp/src/DataStorm/TopicI.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ using namespace DataStormContract;

namespace
{
static Topic::Updater noOpUpdater =
static Topic::Updater noOpUpdater = // NOLINT:cert-err58-cpp
[](const shared_ptr<Sample>& previous, const shared_ptr<Sample>& next, const Ice::CommunicatorPtr&)
{ next->setValue(previous); };

Expand All @@ -38,7 +38,7 @@ namespace

bool match(const shared_ptr<Filterable>&) const final { return true; }
};
const auto alwaysMatchFilter = make_shared<AlwaysMatchFilter>();
const auto alwaysMatchFilter = make_shared<AlwaysMatchFilter>(); // NOLINT:cert-err58-cpp

DataStorm::ClearHistoryPolicy parseClearHistory(const std::string& value)
{
Expand Down
5 changes: 3 additions & 2 deletions cpp/src/DataStorm/TraceUtil.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
#include "SessionI.h"
#include "TopicI.h"

namespace std
// TODO: explain why we need to use namespace std here.
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is for ADL?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably, but it's not immediately clear to me.

namespace std // NOLINT:cert-dcl58-cpp
{
template<typename T> inline std::ostream& operator<<(std::ostream& os, const std::vector<T>& p)
{
Expand Down Expand Up @@ -205,7 +206,7 @@ namespace DataStormI
return os;
}

class TraceLevels
class TraceLevels // NOLINT:clang-analyzer-optin.performance.Padding
{
public:
TraceLevels(const Ice::PropertiesPtr&, Ice::LoggerPtr);
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/Glacier2/InstrumentationI.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ namespace
class Attributes : public AttributeResolverT<SessionHelper>
{
public:
Attributes()
Attributes() noexcept
{
add("parent", &SessionHelper::getInstanceName);
add("id", &SessionHelper::getId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,11 @@ namespace
{
#if defined(__APPLE__) || defined(_WIN32)

// NOLINTBEGIN:cert-err58-cpp
Copy link
Member

Choose a reason for hiding this comment

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

Can we just get rid of the constants? They're only used in this function. and we make a copy all the time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. In simple cases like these, we should use const char* const

const string padBytes0 = "";
const string padBytes1 = "=";
const string padBytes2 = "==";
// NOLINTEND

inline string paddingBytes(size_t length)
{
Expand Down
11 changes: 7 additions & 4 deletions cpp/src/Ice/ArgVector.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,13 @@ IceInternal::ArgVector::ArgVector(const ArgVector& rhs)
IceInternal::ArgVector&
IceInternal::ArgVector::operator=(const ArgVector& rhs)
{
delete[] argv;
argv = nullptr;
_args = rhs._args;
setupArgcArgv();
if (this != &rhs)
{
delete[] argv;
argv = nullptr;
_args = rhs._args;
setupArgcArgv();
}
return *this;
}

Expand Down
4 changes: 2 additions & 2 deletions cpp/src/Ice/CtrlCHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -205,12 +205,12 @@ CtrlCHandler::~CtrlCHandler()
//
void* status = nullptr;
# ifndef NDEBUG
int rc = pthread_kill(_tid, SIGTERM);
int rc = pthread_kill(_tid, SIGTERM); // NOLINT:cert-pos44-c
assert(rc == 0);
rc = pthread_join(_tid, &status);
assert(rc == 0);
# else
pthread_kill(_tid, SIGTERM);
pthread_kill(_tid, SIGTERM); // NOLINT:cert-pos44-c
pthread_join(_tid, &status);
# endif
}
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/Ice/Exception.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
// For UINTPTR_MAX on Ubuntu Precise
//
#ifndef __STDC_LIMIT_MACROS
# define __STDC_LIMIT_MACROS
# define __STDC_LIMIT_MACROS // NOLINT
Copy link
Member

Choose a reason for hiding this comment

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

Ditto. What doesn't the linter like here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't like that we define a __XXX symbol.

#endif

#include "Ice/Exception.h"
Expand Down
11 changes: 0 additions & 11 deletions cpp/src/Ice/FileUtil.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,6 @@

using namespace std;

namespace IceInternal
{
#ifdef _WIN32
const string pathsep = ";";
const string separator = "\\";
#else
const string pathsep = ":";
const string separator = "/";
#endif
}

//
// Determine if path is an absolute path
//
Expand Down
7 changes: 5 additions & 2 deletions cpp/src/Ice/FileUtil.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,11 @@

namespace IceInternal
{
extern const ICE_API std::string pathsep;
extern const ICE_API std::string separator;
#ifdef _WIN32
const char* const separator = "\\";
#else
const char* const separator = "/";
#endif

//
// Determine if path is an absolute path.
Expand Down
4 changes: 3 additions & 1 deletion cpp/src/Ice/HttpParser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ namespace
// Convert a byte array to a string
string bytesToString(const byte* begin, const byte* end)
{
return string(reinterpret_cast<const char*>(begin), reinterpret_cast<const char*>(end));
assert(begin);
assert(end);
return string{reinterpret_cast<const char*>(begin), reinterpret_cast<const char*>(end)};
}
}

Expand Down
2 changes: 1 addition & 1 deletion cpp/src/Ice/Instance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ namespace
class Init
{
public:
Init()
Init() noexcept
{
// Although probably not necessary here, we consistently lock
// staticMutex before accessing instanceList.
Expand Down
14 changes: 7 additions & 7 deletions cpp/src/Ice/InstrumentationI.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ namespace
class Attributes : public AttributeResolverT<ConnectionHelper>
{
public:
Attributes()
Attributes() noexcept
{
add("parent", &ConnectionHelper::getParent);
add("id", &ConnectionHelper::getId);
Expand Down Expand Up @@ -182,7 +182,7 @@ namespace
class Attributes : public AttributeResolverT<DispatchHelper>
{
public:
Attributes()
Attributes() noexcept
{
add("parent", &DispatchHelper::getParent);
add("id", &DispatchHelper::getId);
Expand Down Expand Up @@ -287,7 +287,7 @@ namespace
class Attributes : public AttributeResolverT<InvocationHelper>
{
public:
Attributes()
Attributes() noexcept
{
add("parent", &InvocationHelper::getParent);
add("id", &InvocationHelper::getId);
Expand Down Expand Up @@ -417,7 +417,7 @@ namespace
class Attributes : public AttributeResolverT<RemoteInvocationHelper>
{
public:
Attributes()
Attributes() noexcept
{
add("parent", &RemoteInvocationHelper::getParent);
add("id", &RemoteInvocationHelper::getId);
Expand Down Expand Up @@ -502,7 +502,7 @@ namespace
class Attributes : public AttributeResolverT<CollocatedInvocationHelper>
{
public:
Attributes()
Attributes() noexcept
{
add("parent", &CollocatedInvocationHelper::getParent);
add("id", &CollocatedInvocationHelper::getId);
Expand Down Expand Up @@ -546,7 +546,7 @@ namespace
class Attributes : public AttributeResolverT<ThreadHelper>
{
public:
Attributes()
Attributes() noexcept
{
add("parent", &ThreadHelper::_parent);
add("id", &ThreadHelper::_id);
Expand Down Expand Up @@ -585,7 +585,7 @@ namespace
class Attributes : public AttributeResolverT<EndpointHelper>
{
public:
Attributes()
Attributes() noexcept
{
add("parent", &EndpointHelper::getParent);
add("id", &EndpointHelper::getId);
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/Ice/LoggerI.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ namespace
mutex outputMutex;

// Timeout after which rename will be attempted in case of failures renaming files. That is set to 5 minutes.
const chrono::minutes retryTimeout = chrono::minutes(5);
const chrono::minutes retryTimeout = chrono::minutes(5); // NOLINT:cert-err58-cpp
}

Ice::LoggerI::LoggerI(string prefix, string file, bool convert, size_t sizeMax)
Expand Down
1 change: 1 addition & 0 deletions cpp/src/Ice/MetricsAdminI.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ using namespace IceMX;
namespace
{
const string suffixes[] = {
// NOLINT:cert-err58-cpp
"Disabled",
"GroupBy",
"Accept.*",
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/Ice/OpaqueEndpointI.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ using namespace IceInternal;

namespace
{
static string opaqueEndpointProtocol = "opaque";
static string opaqueEndpointProtocol = "opaque"; // NOLINT:cert-err58-cpp
static string opaqueEndpointConnectionId;
}

Expand Down
10 changes: 5 additions & 5 deletions cpp/src/Ice/Reference.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,11 @@ namespace IceInternal

enum Mode
{
ModeTwoway,
ModeOneway,
ModeBatchOneway,
ModeDatagram,
ModeBatchDatagram,
ModeTwoway = 0,
ModeOneway = 1,
ModeBatchOneway = 2,
ModeDatagram = 3,
ModeBatchDatagram = 4,
ModeLast = ModeBatchDatagram
};

Expand Down
2 changes: 1 addition & 1 deletion cpp/src/Ice/RegisterPluginsInit.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace IceInternal
class RegisterPluginsInit
{
public:
RegisterPluginsInit();
RegisterPluginsInit() noexcept;
};
}

Expand Down
2 changes: 1 addition & 1 deletion cpp/src/Ice/RegisterPluginsInit_all.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ extern "C"
Ice::Plugin* createIceSSL(const Ice::CommunicatorPtr&, const std::string&, const Ice::StringSeq&);
}

IceInternal::RegisterPluginsInit::RegisterPluginsInit()
IceInternal::RegisterPluginsInit::RegisterPluginsInit() noexcept
{
Ice::registerPluginFactory("IceTCP", createIceTCP, true);
Ice::registerPluginFactory("IceSSL", createIceSSL, true);
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/Ice/RegisterPluginsInit_min.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ extern "C"
Ice::Plugin* createIceSSL(const Ice::CommunicatorPtr&, const std::string&, const Ice::StringSeq&);
}

IceInternal::RegisterPluginsInit::RegisterPluginsInit()
IceInternal::RegisterPluginsInit::RegisterPluginsInit() noexcept
{
Ice::registerPluginFactory("IceTCP", createIceTCP, true);
Ice::registerPluginFactory("IceSSL", createIceSSL, true);
Expand Down
4 changes: 2 additions & 2 deletions cpp/src/Ice/SSL/RFC2253.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ namespace
// See RFC 2253 and RFC 1779.
//

string special = ",=+<>#;";
string hexvalid = "0123456789abcdefABCDEF";
const string special = ",=+<>#;"; // NOLINT:cert-err58-cpp
const string hexvalid = "0123456789abcdefABCDEF"; // NOLINT:cert-err58-cpp
}

static char unescapeHex(const string&, size_t);
Expand Down
Loading
Loading