This covers both cases where the inner class is defined in the declaration of a
non-static data member, as needed by clang-cl for
> bridges/source/cpp_uno/msvc_win32_x86-64/except.cxx(799,19): error: unused class member [loplugin:unusedmember]
> PVOID pExceptionObject;
> ~~~~~~^~~~~~~~~~~~~~~~
> bridges/source/cpp_uno/msvc_win32_x86-64/except.cxx(801,19): error: unused class member [loplugin:unusedmember]
> PVOID pThrowImageBase;
> ~~~~~~^~~~~~~~~~~~~~~
as well as anonymous structs (even if there appears to be no need for that
across the LO code base; they are part of C11 and a widely supported C++
extension).
The newly added TODO will eventually need fixing, but I didn't find a way in
Clang to tell whether a RecordDecl is defined as part of a FieldDecl, and the
issue appears to be of no practical relevance for the current LO code base.
Change-Id: I08cecddb9b4a70c3ca480d2d2ab1ea4f198011b2
Reviewed-on: https://gerrit.libreoffice.org/84967
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
With clang-cl on Windows, where CPPU_GCC3_ALIGN expands to nothing, there were
warnings about unsued Char1::c1 and Char2::c2 in cppu/source/uno/check.cxx
(which were suppressed on Linux thanks to CPPU_GCC3_ALIGN(T) making the plugin's
"layout heuristic" kick in for T). But Char1 and Char2 are also (indirect)
bases of Char3, for which the heuristic kicks in thanks to a sizeof(Char3)
expression. Extending the heuristic to base classes seems like a good
improvement anyway, and happens to make the unwanted warnings on Windows go
away.
Change-Id: I1fac3104aa7a1745ccf2f23972953eaacbd45104
Reviewed-on: https://gerrit.libreoffice.org/84927
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
...as exploited in the change of LockingGuard
(extensions/source/activex/SOActiveX.cxx) in
55e596956e56b175ab17b682e7c8ac7daeb9289a "loplugin:external (clang-cl)"
Change-Id: I599ac72b68651fc733dd37c95ce9105c175fee9e
Reviewed-on: https://gerrit.libreoffice.org/83995
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
* See comment at head of compilerplugins/clang/unusedmember.cxx for description.
* Moved isAllRelevantCodeDefined from loplugin:fakebool to PluginHandler for
reuse. (Made it a member function so that it can reuse its two
RecordCompleteMap instances across different loplugins. Probably safer
lifecycle-wise to have them as PluginHandler members than to have them as
static local variables in function isAllRelevantCodeDefined.)
* Need Plugin::ignoreLocation overload for TypeLoc now, thanks to
UnusedMember::VisitElaboratedTypeLoc.
* UETT_PreferredAlignOf was split off UETT_AlignOf with <https://github.com/
llvm/llvm-project/commit/6822bd79ac43f267613f1615bf60407103e24dba> "PR26547:
alignof should return ABI alignment, not preferred alignment".
* RecursiveASTVisitor::TraverseAlignedAttr traverses into the attribute's
argument only since <https://github.com/llvm/llvm-project/commit/
f26d551387f032e05e5e6551605b150f38c3f5b2> "Do not look through pack
expansions when looking for unexpanded parameter packs".
Change-Id: Ic2702b03d4567fa2533333766de7920f3c524a69
Reviewed-on: https://gerrit.libreoffice.org/84416
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
...following up on 4f8a744c4fcf2c69462af19bd807fee32413158d "Make
loplugin:unnecessaryparen treat member expressions consistently"
Change-Id: I444d2995e88990c3c6fa2b912ef68032daf2cad9
Reviewed-on: https://gerrit.libreoffice.org/84112
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
Stumbled across a warning starting to get emitted for some
!(p->x)
when I temporarily changed x from boost::optional (which has a member operator!)
to std::optional (which instead implicitly uses a member conversion operator to
bool). (That is, for the new
static int foo3(Foo2 & foo) {
(void) !(foo.p);
(void) !(foo.b);
(void) !(foo.n);
return (foo.p) ? 1 : 0;
}
test, the first, third, and fourth body lines never warned, while the second one
erroneously warned without this fix.)
Change-Id: I60f6941aaa3a73db0f1373c954e47aa68d703170
Reviewed-on: https://gerrit.libreoffice.org/84079
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
...following up on 314f15bff08b76bf96acf99141776ef64d2f1355 "Extend
loplugin:external to warn about enums".
Cases where free functions were moved into an unnamed namespace along with a
class, to not break ADL, are in:
filter/source/svg/svgexport.cxx
sc/source/filter/excel/xelink.cxx
sc/source/filter/excel/xilink.cxx
svx/source/sdr/contact/viewobjectcontactofunocontrol.cxx
All other free functions mentioning moved classes appear to be harmless and not
give rise to (silent, even) ADL breakage. (One remaining TODO in
compilerplugins/clang/external.cxx is that derived classes are not covered by
computeAffectedTypes, even though they could also be affected by ADL-breakage---
but don't seem to be in any acutal case across the code base.)
For friend declarations using elaborate type specifiers, like
class C1 {};
class C2 { friend class C1; };
* If C2 (but not C1) is moved into an unnamed namespace, the friend declaration
must be changed to not use an elaborate type specifier (i.e., "friend C1;"; see
C++17 [namespace.memdef]/3: "If the name in a friend declaration is neither
qualified nor a template-id and the declaration is a function or an
elaborated-type-specifier, the lookup to determine whether the entity has been
previously declared shall not consider any scopes outside the innermost
enclosing namespace.")
* If C1 (but not C2) is moved into an unnamed namespace, the friend declaration
must be changed too, see <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71882>
"elaborated-type-specifier friend not looked up in unnamed namespace".
Apart from that, to keep changes simple and mostly mechanical (which should help
avoid regressions), out-of-line definitions of class members have been left in
the enclosing (named) namespace. But explicit specializations of class
templates had to be moved into the unnamed namespace to appease
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92598> "explicit specialization of
template from unnamed namespace using unqualified-id in enclosing namespace".
Also, accompanying declarations (of e.g. typedefs or static variables) that
could arguably be moved into the unnamed namespace too have been left alone.
And in some cases, mention of affected types in blacklists in other loplugins
needed to be adapted.
And sc/qa/unit/mark_test.cxx uses a hack of including other .cxx, one of which
is sc/source/core/data/segmenttree.cxx where e.g. ScFlatUInt16SegmentsImpl is
not moved into an unnamed namespace (because it is declared in
sc/inc/segmenttree.hxx), but its base ScFlatSegmentsImpl is. GCC warns about
such combinations with enabled-by-default -Wsubobject-linkage, but "The compiler
doesn’t give this warning for types defined in the main .C file, as those are
unlikely to have multiple definitions."
(<https://gcc.gnu.org/onlinedocs/gcc-9.2.0/gcc/Warning-Options.html>) The
warned-about classes also don't have multiple definitions in the given test, so
disable the warning when including the .cxx.
Change-Id: Ib694094c0d8168be68f8fe90dfd0acbb66a3f1e4
Reviewed-on: https://gerrit.libreoffice.org/83239
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
...in non-deduced context. That means that the necessary cast in
84322944980f6e2f9d4a531de7a6803797156968 "Simplify sequence initialization"
(cf. <https://gerrit.libreoffice.org/#/c/82974/4/>) no longer causes a false
positive, and doesn't need to use comphelper::OUStringLiteralList.
Change-Id: I788da61cc0be82d2166653760e527bb18e366c99
Reviewed-on: https://gerrit.libreoffice.org/83291
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
To mitigate the dangers of silently breaking ADL when moving enums into unnamed
namespaces (see the commit message of 206b5b2661be37efdff3c6aedb6f248c4636be79
"New loplugin:external"), note all functions that are affected. (The plan is to
extend loplugin:external further to also warn about classes and class templates,
and the code to identify affected functions already takes that into account, so
some parts of that code are not actually relevant for enums.)
But it appears that none of the functions that are actually affected by the
changes in this commit relied on being found through ADL, so no adaptions were
necessary for them.
(clang::DeclContext::collectAllContexts is non-const, which recursively means
that External's Visit... functions must take non-const Decl*. Which required
compilerplugins/clang/sharedvisitor/analyzer.cxx to be generalized to support
such Visit... functions with non-const Decl* parameters.)
Change-Id: Ia215291402bf850d43defdab3cff4db5b270d1bd
Reviewed-on: https://gerrit.libreoffice.org/83001
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
...and add a minimal test for it
Change-Id: Ia6c61e41a7e60fd01c639e893c34bd9d215c1513
Reviewed-on: https://gerrit.libreoffice.org/82983
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
...not just those that came before
Change-Id: Ib10ca34cb2ee63124999d56bb00d94f000f33ea1
Reviewed-on: https://gerrit.libreoffice.org/82782
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
...and improve diagnostics a bit
Change-Id: I3233aa1752620ddbe6fbeff93b15565921f0bc2e
Reviewed-on: https://gerrit.libreoffice.org/82767
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
...to: "Find implicit conversions from non-'bool' constants (e.g., 'sal_False')
to 'bool'".
Due to how FALSE is defined as just
#define FALSE (0)
(i.e., a literal of type 'int') but TRUE is defined as
#define TRUE (!FALSE)
(i.e., an implicit conversion from 'int' to 'bool') in GLib (see the comment in
ConstToBool::VisitImplicitCastExpr), we get more warnings about uses of 'TRUE'
than of 'FALSE'. For example, in libreofficekit/source/gtk/lokdocview.cxx there
is a warning about the 'TRUE' in
g_main_context_iteration(nullptr, TRUE);
but not about the 'FALSE' in
g_main_context_iteration(nullptr, FALSE);
(where the parameter of 'g_main_context_iteration' is of type 'gboolean'). Lets
live with that asymmetry for now...
(Besides the issues addressed directly in this commit, it also found the two
bogus asserts at 7e09d08807b5ba2fd8b9831557752a415bdad562 "Fix useless
assert(true) (which would never fire)" and
122a0be8ae480473bd1d7f35e197a2529f4621e3 "Fix useless assert(true) (which would
never fire)", plus 5f0d6df7f57ae281fe161e61c7f25d67453fddd2 "Use two-argument
form of static_assert".)
Change-Id: Id77322de9f94b85a7b65608a03e0e9865d14467b
Reviewed-on: https://gerrit.libreoffice.org/82667
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
...checking for unnecessary uses of more "fake bool" types.
In the past, some of the checks involving the types of variables or data
members, or the return types of functions, issued warnings that required
surrounding code to be changed too (e.g., when changing the signature of a
function whose address was taken). These checks have been tightened now to not
warn in such cases (which avoids warnings that require changes to additional
code, or changes that might even be impossible to make, at the cost of being
less aggressive about removing all unnecessary uses of those "fake bool" types).
Change-Id: I70eb75039817cda34ed611387ee27dc5f36a3e2e
Reviewed-on: https://gerrit.libreoffice.org/82554
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
so I don't read the "then" block as being a sequential statements
Change-Id: Ib2004acd3518bd4ebd2246f02a26c2c0a8bbab4c
Reviewed-on: https://gerrit.libreoffice.org/82069
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
See comments at <https://gerrit.libreoffice.org/#/c/81958/> "Only initialize
function pointer once" for a case where such a false warning caused trouble (in
a lambda with deduced return type and multiple return statements).
Change-Id: I64b0b8c45bd3d2a6075e336c361ec778fa0da481
Reviewed-on: https://gerrit.libreoffice.org/82034
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
...to find StringLiteral on the RHS of +=. Which revealed that the
VisitCompoundStmt/checkForCompoundAssign logic needed to be fixed, too, so that
s += side_effect();
s += "literal";
s += side_effect();
only gets combined to
s += side_effect() + "literal";
s += side_effect();
and not all the way to
s += side_effect() + "literal" + side_effect();
Change-Id: I432e3458b933a7d0ad6141c747b675cc8b0f0ba4
Reviewed-on: https://gerrit.libreoffice.org/81804
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
if one side of the expression is a compile-time-constant, we don't need
to worry about side-effects on the other side
Change-Id: Iee71ea51b327ef244bf39f128f921ac325d74e2b
Reviewed-on: https://gerrit.libreoffice.org/81589
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
...similar to OUStringChar, to be used in string concatenation expressions. And
enable the corresponding loplugin:stringadd check, and fix its findings.
Change-Id: I35ebb2253ba82bda6c98ae6ebd2ad4f27cf9abf9
Reviewed-on: https://gerrit.libreoffice.org/81456
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
...which is reported now by Clang 10 trunk with -std=c++2a:
> cui/source/tabpages/tpline.cxx:481:80: error: use of overloaded operator '==' is ambiguous (with operand types 'const XLineEndItem' and 'XLineStartItem')
> if( pItem && ( !pOld || !( *static_cast<const XLineEndItem*>(pOld) == *pItem ) ) )
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~
> include/svx/xlnstit.hxx:43:29: note: candidate function (with reversed parameter order)
> virtual bool operator==(const SfxPoolItem& rItem) const override;
> ^
> include/svx/xlnedit.hxx:43:29: note: candidate function
> virtual bool operator==(const SfxPoolItem& rItem) const override;
> ^
But the base SfxPoolItem::operator == is virtual anyway, so no need to cast
pOld to a derived type.
And once the expression is changed to
!( *pOld == *pItem )
loplugin:simplifybool would kick in, but only with old compilers. So update
loplugin:simplifybool to also kick in on that with latest Clang trunk with
-std=c++2a, and simplify the expression accordingly.
Change-Id: I3de9175b30d8645ed7a52f87cfac320144576cc8
Reviewed-on: https://gerrit.libreoffice.org/81203
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
...like the one manually found at 06845c14a17146b0ec50dd3704b6af9ee13927bc "This
should have become a UTF-16 string literal". (No further misuses were found
across the code base.)
Change-Id: I0b604bdaaa38bd9248440ff7bd7bf0545fc6426a
Reviewed-on: https://gerrit.libreoffice.org/81250
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
and create conversion methods on *StringBuffer to make this work
Change-Id: I3cf5ee3e139826168894b46eff8ee4bcde00cb7e
Reviewed-on: https://gerrit.libreoffice.org/80949
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
and extend O*StringView to have a constructor that takes a pointer and a
length
Change-Id: I6120e96280f030757e855a6596efdae438b7e1e8
Reviewed-on: https://gerrit.libreoffice.org/80872
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
...after 9b5dad13b56bdde7c40970351af3da3a2c3c9350 "loplugin:stringadd look for
unnecessary temporaries". There was no reason to check for implicit
MaterializeTemporaryExpr instead of explicitly written CXXFunctionalCastExpr,
and checking for the latter makes it easier to report the casted-from type,
which gives useful information how to change code that exhibits the warning.
See the comments at <https://gerrit.libreoffice.org/#/c/80724/>
"loplugin:stringadd look for unnecessary temporaries" for details.
(And while at it, remove some commented-out debug code that is probably no
longer relevant now.)
Change-Id: I7d4cab85432885d617dd7114c75163c1eb376fc2
Reviewed-on: https://gerrit.libreoffice.org/80823
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
look for OUStringBuffer append sequences that can be turned
into creating an OUString with + operations
Change-Id: Ica840dc096000307b4a105fb4d9ec7588a15ade6
Reviewed-on: https://gerrit.libreoffice.org/80809
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
...to find matches of
... << s.getStr()
(for the rtl string classes) that can be written as just
... << s
Some notes:
* The OUStringToOString(..., RTL_TEXTENCODING_UTF8) is left explicit in
desktop/source/app/crashreport.cxx (even though that would also be done
internally by the "<< OUString" operator) to clarify that these values are
written out as UTF-8 (and not as what that operator << happens to use, which
just also happens to be UTF-8).
* OUSTRING_TO_CSTR (include/oox/helper/helper.hxx) is no longer used now.
* Just don't bother to use osl_getThreadTextEncoding() in the SAL_WARN in
lingucomponent/source/hyphenator/hyphen/hyphenimp.cxx.
* The toUtf8() in the SAL_DEBUG in pyuno/source/module/pyuno_module.cxx can just
go, too.
Change-Id: I4602f0379ef816bff310f1e51b57c56b7e3f0136
Reviewed-on: https://gerrit.libreoffice.org/80762
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
which defeat the *StringConcat optimisation.
Also make StringConcat conversions treat a nullptr as an empty string,
to match the O*String(char*) constructors.
Change-Id: If45f5b4b6a535c97bfeeacd9ec472a7603a52e5b
Reviewed-on: https://gerrit.libreoffice.org/80724
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Required a workaround for loplugin:indentation, until
<https://reviews.llvm.org/D68581> "Include leading attributes in DeclStmt's
SourceRange" lands in Clang.
Change-Id: I7192969d40fa4c50bbd603d059532b9344865248
Reviewed-on: https://gerrit.libreoffice.org/80596
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
...when the get member function is implemented in a base class, as happens for
std::shared_ptr in libstdc++ (where it is implemented in base __shared_ptr; see
also 7d361e96c9ea822790db21806e9fc05279423833 "loplugin:redundantpointerops").
And while at it, check for precisely the classes we are interested in (for which
we know the semantics of get and operator*), rather than any classes whose
unqualified names happen to match.
Change-Id: I0c85ba46f191a2ee038c8175d979aa0c1be765cd
Reviewed-on: https://gerrit.libreoffice.org/80585
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
...to complement and improve upon GCC's -Wclass-memaccess. See the comment at
the head of compilerplugins/clang/classmemaccess.cxx for details. (It is not
yet clear to me whether we would want to get this upstreamed into Clang.)
35d21e4bf6f66b3bbc7a44fcf184cb721b524a94 "Remove redundant memsets" was a case
that benefited from looking through toplevel casts to void*. (Though the code
in include/basic/sbxvar.hxx needs a slightly more verbose way to deliberately
silence the warning now.)
d03041e19215592f21ba1222d3cfa29e1f94260a "Drop bogus memsets" is one example of
various cases that GCC -Wclass-memaccess failed to catch due to the use of array
instead of pointer types.
Change-Id: I6a9bfc34e3536834af35fdf4fb7ceeb31f31f8c0
Reviewed-on: https://gerrit.libreoffice.org/80421
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
look for places where we can replace sequential additions to
OUString/OString with one concatentation (i.e. +) expression, which is
more efficient
Change-Id: I64d91328bf64828d8328b1cad9e90953c0a75663
Reviewed-on: https://gerrit.libreoffice.org/79406
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Apply the constmethod plugin, but only to accessor-type methods, e.g.
IsFoo(), GetBar(), etc, where we can be sure of that
constifying is a reasonable thing to do.
Change-Id: Ibc97f5f359a0992dd1ce2d66f0189f8a0a43d98a
Reviewed-on: https://gerrit.libreoffice.org/74269
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
"make range var const" sounded to me like it talked about the variable declared
in the for-range-declaration, not a variable referenced in the for-range-
initializer
Change-Id: Ie777e1374ead7f37c8efb022cd87e980d2ee9810
Reviewed-on: https://gerrit.libreoffice.org/79563
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
When I did the fast string concatenation, I didn't add any support
for number(), which simply returned a O(U)String, and so it did
the extra allocation/deallocation, although that could be avoided.
In order to support this, number() now returns a special temporary
return type, similarly to O(U)StringConcat, which allows delaying
the concatenation the same way.
Also similarly, the change of the return type in some cases requires
explicit cast to the actual string type. Usage of OString::getStr()
is so extensive in the codebase that I actually added it to the helper
class, after that it's only relatively few cases.
Change-Id: Iba6e158010e1e458089698c426803052b6f46031
Reviewed-on: https://gerrit.libreoffice.org/78873
Tested-by: Jenkins
Reviewed-by: Luboš Luňák <l.lunak@collabora.com>
Something like auto str = "string" + OUString::number( 10 ); will
not be OUString but actually rtl::OUStringConcat (which gets implicitly
converted to OUString, but not with auto). Since those refer to temporaries
from the expression, they should not outlive the expression.
Change-Id: Ib4cde4b38befb3d49927d0cf01c52ebb2d36df89
Reviewed-on: https://gerrit.libreoffice.org/78830
Tested-by: Jenkins
Reviewed-by: Luboš Luňák <l.lunak@collabora.com>
Where the problem was benign and the class was not extended, I marked
the class as final.
Where the problem was benign and the class was extended, I marked the
relevant callee methods as final.
Other cases were excluded in the plugin.
Change-Id: Idb762fb2206af4e8b534aa35ff77f8368c7909bc
Reviewed-on: https://gerrit.libreoffice.org/79089
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
