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 lints in generated C++ code #3262

Merged
merged 4 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
8 changes: 5 additions & 3 deletions .clang-tidy
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,16 @@ Checks:
modernize-*,
-modernize-avoid-c-arrays,
-modernize-use-trailing-return-type,
-modernize-concat-nested-namespaces,
performance-*,
-performance-avoid-endl
'
# WarningsAsErrors: '*'
HeaderFilterRegex: ''
WarningsAsErrors: '*'
HeaderFilterRegex: '.*'
ExcludeHeaderFilterRegex: 'include/.*'
UseColor: true
FormatStyle: 'file'
ExtraArgs: ['-std=c++17']
ExtraArgs: ['-std=c++20']
Copy link
Member Author

Choose a reason for hiding this comment

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

We use C++20 types (std::span) for a test in C++20 mode; clang-tidy doesn't like -std=c++17 for that test.

CheckOptions:
modernize-use-nullptr.NullMacros: 'NULL'
# std::exception_ptr is a cheap to copy, pointer-like type; we pass it by value all the time.
Expand Down
10 changes: 5 additions & 5 deletions cpp/include/Ice/Exception.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,14 @@ namespace Ice
* Returns the error message of this exception.
* @return The error message.
*/
const char* what() const noexcept override;
[[nodiscard]] const char* what() const noexcept override;

/**
* Returns the type ID of this exception. This corresponds to the Slice type ID for Slice-defined exceptions,
* and to a similar fully scoped name for other exceptions. For example "::Ice::CommunicatorDestroyedException".
* @return The type ID of this exception
*/
virtual const char* ice_id() const noexcept = 0;
[[nodiscard]] virtual const char* ice_id() const noexcept = 0;

/**
* Outputs a description of this exception to a stream. This function is called by operator<<(std::ostream&,
Expand All @@ -76,19 +76,19 @@ namespace Ice
* Returns the name of the file where this exception was constructed.
* @return The file name.
*/
const char* ice_file() const noexcept;
[[nodiscard]] const char* ice_file() const noexcept;

/**
* Returns the line number where this exception was constructed.
* @return The line number.
*/
int ice_line() const noexcept;
[[nodiscard]] int ice_line() const noexcept;

/**
* Returns the stack trace at the point this exception was constructed.
* @return The stack trace as a string, or an empty string if stack trace collection is not enabled.
*/
std::string ice_stackTrace() const;
[[nodiscard]] std::string ice_stackTrace() const;

/**
* Enables the collection of stack traces for exceptions. On Windows, calling this function more than once is
Expand Down
4 changes: 2 additions & 2 deletions cpp/include/Ice/Object.h
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ namespace Ice
* @param current The Current object for the invocation.
* @return The Slice type IDs of the interfaces supported by this object, in alphabetical order.
*/
virtual std::vector<std::string> ice_ids(const Current& current) const;
[[nodiscard]] virtual std::vector<std::string> ice_ids(const Current& current) const;
/// \cond INTERNAL
void _iceD_ice_ids(IncomingRequest&, std::function<void(OutgoingResponse)>);
/// \endcond
Expand All @@ -87,7 +87,7 @@ namespace Ice
* @param current The Current object for the invocation.
* @return The Slice type ID of the most-derived interface.
*/
virtual std::string ice_id(const Current& current) const;
[[nodiscard]] virtual std::string ice_id(const Current& current) const;
/// \cond INTERNAL
void _iceD_ice_id(IncomingRequest&, std::function<void(OutgoingResponse)>);
/// \endcond
Expand Down
2 changes: 1 addition & 1 deletion cpp/include/Ice/UserException.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ namespace Ice
virtual void _write(OutputStream*) const;
virtual void _read(InputStream*);

virtual bool _usesClasses() const;
[[nodiscard]] virtual bool _usesClasses() const;
/// \endcond

protected:
Expand Down
6 changes: 3 additions & 3 deletions cpp/include/Ice/Value.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,14 @@ namespace Ice
* Creates a shallow polymorphic copy of this instance.
* @return The cloned value.
*/
ValuePtr ice_clone() const { return _iceCloneImpl(); }
[[nodiscard]] ValuePtr ice_clone() const { return _iceCloneImpl(); }

/**
* Obtains the sliced data associated with this instance.
* @return The sliced data if the value has a preserved-slice base class and has been sliced during
* unmarshaling of the value, nil otherwise.
*/
SlicedDataPtr ice_getSlicedData() const;
[[nodiscard]] SlicedDataPtr ice_getSlicedData() const;

/// \cond STREAM
virtual void _iceWrite(Ice::OutputStream*) const;
Expand All @@ -85,7 +85,7 @@ namespace Ice
static std::shared_ptr<T> clone(const T& other) { return std::make_shared<CloneEnabler>(other); }
};

virtual ValuePtr _iceCloneImpl() const;
[[nodiscard]] virtual ValuePtr _iceCloneImpl() const;
/// \endcond

/// \cond STREAM
Expand Down
46 changes: 33 additions & 13 deletions cpp/src/slice2cpp/Gen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1111,9 +1111,14 @@ Slice::Gen::ForwardDeclVisitor::visitEnum(const EnumPtr& p)
H << "class ";
}
H << getDeprecatedAttribute(p) << fixKwd(p->name());
if (!unscoped && p->maxValue() <= 0xFF)
if (!unscoped)
Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately we have no test for unscoped: we should either test it or remove it.

{
H << " : ::std::uint8_t";
H << " : ::std::" << (p->maxValue() <= numeric_limits<uint8_t>::max() ? "uint8_t" : "int32_t");

if (p->maxValue() > numeric_limits<uint8_t>::max() && p->maxValue() <= numeric_limits<int16_t>::max())
{
H << " // NOLINT:performance-enum-size";
}
}
H << sb;

Expand Down Expand Up @@ -1221,7 +1226,12 @@ Slice::Gen::ForwardDeclVisitor::visitConst(const ConstPtr& p)
<< typeToString(p->type(), false, scope, p->typeMetadata(), _useWstring) << " " << fixKwd(p->name()) << " "
<< getDeprecatedAttribute(p) << "= ";
writeConstantValue(H, p->type(), p->valueType(), p->value(), _useWstring, p->typeMetadata(), scope);
H << ';' << sp;
H << ';';
if (!isConstexprType(p->type()))
bernardnormier marked this conversation as resolved.
Show resolved Hide resolved
{
H << " // NOLINT:cert-err58-cpp";
}
H << sp;
}

Slice::Gen::DefaultFactoryVisitor::DefaultFactoryVisitor(Output& c) : C(c), _factoryTableInitDone(false) {}
Expand Down Expand Up @@ -1543,6 +1553,10 @@ Slice::Gen::ProxyVisitor::visitOperation(const OperationPtr& p)
}
H << nl << deprecatedAttribute << retS << ' ' << fixKwd(name) << spar << paramsDecl << contextDecl << epar
<< " const;";
if (ret)
{
H << " // NOLINT:modernize-use-nodiscard";
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't generate a [[nodiscard]] for sync proxy functions returning something.

}

C << sp;
C << nl << retSImpl << nl << scoped << fixKwd(name) << spar << paramsImplDecl << "const ::Ice::Context& context"
Expand Down Expand Up @@ -2068,7 +2082,7 @@ Slice::Gen::DataDefVisitor::visitExceptionStart(const ExceptionPtr& p)
C << nl << "return \"" << p->scoped() << "\";";
C << eb;

H << sp << nl << _dllMemberExport << "const char* ice_id() const noexcept override;";
H << sp << nl << _dllMemberExport << "[[nodiscard]] const char* ice_id() const noexcept override;";
C << sp << nl << "const char*" << nl << scoped.substr(2) << "::ice_id() const noexcept";
C << sb;
C << nl << "return ice_staticId();";
Expand All @@ -2084,7 +2098,7 @@ Slice::Gen::DataDefVisitor::visitExceptionStart(const ExceptionPtr& p)
{
H << sp;
H << nl << "/// \\cond STREAM";
H << nl << _dllMemberExport << "bool _usesClasses() const override;";
H << nl << _dllMemberExport << "[[nodiscard]] bool _usesClasses() const override;";
H << nl << "/// \\endcond";

C << sp;
Expand Down Expand Up @@ -2218,7 +2232,7 @@ Slice::Gen::DataDefVisitor::visitClassDefStart(const ClassDefPtr& p)
C << nl << "return \"" << p->scoped() << "\";";
C << eb;

H << sp << nl << _dllMemberExport << "const char* ice_id() const noexcept override;";
H << sp << nl << _dllMemberExport << "[[nodiscard]] const char* ice_id() const noexcept override;";
C << sp << nl << "const char*" << nl << scoped.substr(2) << "::ice_id() const noexcept";
C << sb;
C << nl << "return ice_staticId();";
Expand All @@ -2235,7 +2249,7 @@ Slice::Gen::DataDefVisitor::visitClassDefStart(const ClassDefPtr& p)
H << sp;
H << nl << "/// Creates a shallow polymorphic copy of this instance.";
H << nl << "/// @return The cloned value.";
H << nl << p->name() << "Ptr ice_clone() const { return ::std::static_pointer_cast<" << name
H << nl << "[[nodiscard]] " << p->name() << "Ptr ice_clone() const { return ::std::static_pointer_cast<" << name
<< ">(_iceCloneImpl()); }";

return true;
Expand Down Expand Up @@ -2302,7 +2316,7 @@ Slice::Gen::DataDefVisitor::visitClassDefEnd(const ClassDefPtr& p)
}

H << nl << name << "(const " << name << "&) = default;";
H << sp << nl << _dllMemberExport << "::Ice::ValuePtr _iceCloneImpl() const override;";
H << sp << nl << _dllMemberExport << "[[nodiscard]] ::Ice::ValuePtr _iceCloneImpl() const override;";
C << sp;
C << nl << "::Ice::ValuePtr" << nl << scoped.substr(2) << "::_iceCloneImpl() const";
C << sb;
Expand Down Expand Up @@ -2571,13 +2585,13 @@ Slice::Gen::InterfaceVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
H << nl << "/// Obtains a list of the Slice type IDs representing the interfaces supported by this object.";
H << nl << "/// @param current The Current object for the invocation.";
H << nl << "/// @return A list of fully-scoped type IDs.";
H << nl << "::std::vector<::std::string> ice_ids(const " << getUnqualified("::Ice::Current&", scope)
H << nl << "[[nodiscard]] ::std::vector<::std::string> ice_ids(const " << getUnqualified("::Ice::Current&", scope)
<< " current) const override;";
H << sp;
H << nl << "/// Obtains a Slice type ID representing the most-derived interface supported by this object.";
H << nl << "/// @param current The Current object for the invocation.";
H << nl << "/// @return A fully-scoped type ID.";
H << nl << "::std::string ice_id(const " << getUnqualified("::Ice::Current&", scope) << " current) const override;";
H << nl << "[[nodiscard]] ::std::string ice_id(const " << getUnqualified("::Ice::Current&", scope) << " current) const override;";
H << sp;
H << nl << "/// Obtains the Slice type ID corresponding to this interface.";
H << nl << "/// @return A fully-scoped type ID.";
Expand Down Expand Up @@ -2743,12 +2757,20 @@ Slice::Gen::InterfaceVisitor::visitOperation(const OperationPtr& p)

DocCommentPtr comment = p->parseDocComment(cppLinkFormatter);

string isConst = p->hasMetadata("cpp:const") ? " const" : "";
string noDiscard = "";

if (ret)
{
string typeS = inputTypeToString(ret, p->returnIsOptional(), interfaceScope, p->getMetadata(), _useWstring);
responseParams.push_back(typeS + " " + returnValueParam);
responseParamsDecl.push_back(typeS + " ret");
responseParamsImplDecl.push_back(typeS + " ret");

if (!amd && !isConst.empty())
{
noDiscard = "[[nodiscard]] ";
bernardnormier marked this conversation as resolved.
Show resolved Hide resolved
}
}

string retS;
Expand Down Expand Up @@ -2880,8 +2902,6 @@ Slice::Gen::InterfaceVisitor::visitOperation(const OperationPtr& p)
C << eb;
}

string isConst = p->hasMetadata("cpp:const") ? " const" : "";

string opName = amd ? (name + "Async") : fixKwd(name);

H << sp;
Expand All @@ -2905,7 +2925,7 @@ Slice::Gen::InterfaceVisitor::visitOperation(const OperationPtr& p)
postParams.push_back("@param " + currentParam + " The Current object for the invocation.");
writeOpDocSummary(H, p, comment, pt, true, GenerateDeprecated::No, StringList(), postParams, returns);
}
H << nl << "virtual " << retS << ' ' << opName << spar << params << epar << isConst << " = 0;";
H << nl << noDiscard << "virtual " << retS << ' ' << opName << spar << params << epar << isConst << " = 0;";
H << nl << "/// \\cond INTERNAL";
H << nl << "void _iceD_" << name << "(::Ice::IncomingRequest&, ::std::function<void(::Ice::OutgoingResponse)>)"
<< isConst << ';';
Expand Down
48 changes: 27 additions & 21 deletions cpp/test/Ice/custom/CustomBuffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ namespace Test
template<typename T> class CustomBuffer
{
public:
CustomBuffer() : _buf(nullptr), _count(0) {}
CustomBuffer() = default;

CustomBuffer(const CustomBuffer& o) : _buf(0), _count(o._count)
CustomBuffer(const CustomBuffer& o) : _count(o._count)
{
if (_count > 0)
{
Expand All @@ -31,7 +31,7 @@ namespace Test
}
}

CustomBuffer(CustomBuffer&& o) : _buf(o._buf), _count(o._count)
CustomBuffer(CustomBuffer&& o) noexcept : _buf(o._buf), _count(o._count)
{
o._buf = nullptr;
o._count = 0;
Expand All @@ -41,31 +41,37 @@ namespace Test

CustomBuffer& operator=(const CustomBuffer& o)
{
_count = o._count;
if (_count > 0)
if (this != &o)
{
_buf = new T[_count];
for (size_t i = 0; i < _count; ++i)
_count = o._count;
if (_count > 0)
{
_buf[i] = o._buf[i];
_buf = new T[_count];
for (size_t i = 0; i < _count; ++i)
{
_buf[i] = o._buf[i];
}
}
}
return *this;
}

CustomBuffer& operator=(CustomBuffer&& o)
CustomBuffer& operator=(CustomBuffer&& o) noexcept
{
delete[] _buf;
_buf = o._buf;
_count = o._count;
o._buf = nullptr;
o._count = 0;
if (this != &o)
{
delete[] _buf;
_buf = o._buf;
_count = o._count;
o._buf = nullptr;
o._count = 0;
}
return *this;
}

size_t count() const { return _count; }
[[nodiscard]] size_t count() const { return _count; }

T* get() const { return _buf; }
[[nodiscard]] T* get() const { return _buf; }

void set(T* buf, size_t count)
{
Expand All @@ -79,13 +85,13 @@ namespace Test
_count = count;
for (size_t i = 0; i < count; ++i)
{
_buf[i] = static_cast<T>(rand());
_buf[i] = static_cast<T>(rand()); // NOLINT
}
}

private:
T* _buf;
size_t _count;
T* _buf{nullptr};
size_t _count{0};
};

template<typename T> bool operator!=(const CustomBuffer<T>& lhs, const CustomBuffer<T>& rhs)
Expand Down Expand Up @@ -152,7 +158,7 @@ namespace Ice
{
std::pair<const T*, const T*> a;
stream->read(a);
size_t count = static_cast<size_t>(a.second - a.first);
auto count = static_cast<size_t>(a.second - a.first);
if (count > 0)
{
auto b = new T[count];
Expand All @@ -164,7 +170,7 @@ namespace Ice
}
else
{
v.set(0, 0);
v.set(nullptr, 0);
}
}
};
Expand Down
4 changes: 2 additions & 2 deletions cpp/test/Ice/custom/MyByteSeq.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ MyByteSeq::size() const
}

void
MyByteSeq::swap(MyByteSeq& seq)
MyByteSeq::swap(MyByteSeq& seq) noexcept
{
size_t tmpSize = seq._size;
std::byte* tmpData = seq._data;
Expand Down Expand Up @@ -84,7 +84,7 @@ MyByteSeq::operator=(const MyByteSeq& rhs)
}

MyByteSeq&
MyByteSeq::operator=(MyByteSeq&& rhs)
MyByteSeq::operator=(MyByteSeq&& rhs) noexcept
{
delete[] _data;
_data = nullptr;
Expand Down
Loading
Loading