to look for the
x.get() != null
pattern, which can be simplified to
x
I'll do the
x.get() == nullptr
pattern in a separate patch, to reduce the chances of a mistake
Change-Id: I45e0d178e75359857cdf50d712039cb526016555
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/95354
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
...as discussed as an open TODO in the commit message of
fe6cce01c8 "Fix loplugin:simplifypointertobool for
libstdc++ std::shared_ptr". The necessary changes across the code base have
been done fully automatically with the rewriting plugin on Linux. (All those
changes apparently involve uses of macro arguments wrapped in parentheses in the
macro body, but always in conditionally-converted-to-bool contexts. In other
contexts, such automatic rewriting would add the "bool" to the macro body, which
would be wrong in general, but we apparently get away with that sloppy coding
for now.)
The parenExprs_ stack that fe6cce01c8 had
introduced to treat such (then-undetected, it had turned out) parenthesized
cases now turns out to not be needed after all.
Change-Id: I2021f61c2e2805be7e18b38edf8744d186cac3cb
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/95010
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
...where the get member function is defined on a std::__shared_ptr base class,
so loplugin:simplifypointertobool used to miss those until now. (While e.g.
using libc++ on macOS found those cases.)
366d08f2f6 "new loplugin:simplifypointertobool"
was mistaken in breaking isSmartPointerType(const clang::Type* t) out of
isSmartPointerType(const Expr* e); c874294ad9 "Fix
detection of std::unique_ptr/shared_ptr in loplugin:redundantpointerops" had
introduced that indivisible two-step algorithm on purpose.
The amount of additional hits (on Linux) apparently asked for turning
loplugin:simplifypointertobool into a rewriting plugin. Which in turn showed
that the naive adivce to just "drop the get()" is not sufficient in places that
are not contextually converted to bool, as those places need to be wrapped in a
bool(...) functional cast now. If the expression was already wrapped in
parentheses, those could be reused as part of the functional cast, but
implementing that showed that such cases are not yet found at all by the
existing loplugin:simplifypointertobool. Lets leave that TODO for another
commit.
Besides the changes to compilerplugins/ itself, this change has been generated
fully automatically with the rewriting plugin on Linux.
Change-Id: I83107d6f634fc9ac232986f49044d7017df83e2a
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/94888
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
Tested-by: Jenkins
...where the relevant types' actual names are std::__1::unique_ptr and
std::__1::default_delete
Change-Id: I2c364535061691415703d6ff0f58269cdd192a6b
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/93576
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
to find places where we are converting stuff to unique_ptr
instead of using std::make_shared.
As a bonus, this tends to find places where we are using shared_ptr
where we can instead be using unique_ptr avoiding the locking overhead.
Change-Id: I1b57bbc4a6c766b48bba8e25a55161800e149f62
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/93207
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
...that are equivalent to const_cast or reinterpret_cast, and should arguably
better be written that way for clarity. Drawing inspiration from
<https://reviews.llvm.org/D76572> "Replace `T(x)` with `reinterpret_cast<T>(x)`
everywhere it means reinterpret_cast. No functional change".
Change-Id: I27b8f70d324d32ecba658db4d1c2db03e10d5d3e
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/91086
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
noticed something that wasn't being picked up, wrote some tests,
and found an unhandled case in Plugin::getParentFunctionDecl
Change-Id: I52b4ea273be6614e197392dfc4d6053bbc1704de
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/90141
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
to help me maintain the invariants when updating code to use the
FastParser APIs. One weird invariant is that you need to override
startFastElement or the createFastChildContext will not get called.
Not all of these changes are probably necessary - some of the classes
are never constructured themselves, only their subclasses are
constructed, and their subclasses maintain the invariants, but it is
just easier to scatter a few more startFastElement around
Change-Id: I3f70fb5a1e44c311cf4926fa7b0fcda605709eac
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/89473
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
...after <https://github.com/llvm/llvm-project/commit/
9ce6dc9872be4081fb98f6161c28581e1cbbe7dc> "CWG1423: don't permit implicit
conversion of nullptr_t to bool." (Direct-initialization from std::nullptr_t to
bool is allowed in C++17, but it appears that will be dropped from C++20, see
<https://github.com/cplusplus/draft/commit/
df3e38121431afd9adcf7dce725a670a235463ea> "CWG1781 Converting from nullptr_t to
bool in overload resolution", at which point the new check for initialization of
S::b and the res.isNullPointer() branch in ConstToBool::VisitImplicitCastExpr,
compilerplugins/clang/consttobool.cxx, will probably become moot.)
Change-Id: I99773d13d514d5ba5296843592b740ea949b2b1f
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/88784
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
check indentation of braces in namespace decls,
and the comments that often appear with them.
This is my penance for messing up the indentation with
clang-tidy-modernize-namespaces.
As such I have limited it to new-style namespaces for now,
and the check is off by default.
Change-Id: I4db7f10a81c79bc0eece8f8e3ee564da8bc7f168
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/87723
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
which I wrote to check if there were any other cases where assigning to
a ref local var looks dodgy, after my fixes in
0b113d6ebb, but I didn't find anything, so
just leave this one in store
Change-Id: Ib820924c5e8aa85206730afeb06972ef48231ec5
"Find explicit casts from signed to unsigned integer in comparison against
unsigned integer, where the cast is presumably used to avoid warnings about
signed vs. unsigned comparisons, and could thus be replaced with
o3tl::make_unsigned for clairty." (compilerplugins/clang/unsignedcompare.cxx)
o3tl::make_unsigned requires its argument to be non-negative, and there is a
chance that some original code like
static_cast<sal_uInt32>(n) >= c
used the explicit cast to actually force a (potentially negative) value of
sal_Int32 to be interpreted as an unsigned sal_uInt32, rather than using the
cast to avoid a false "signed vs. unsigned comparison" warning in a case where
n is known to be non-negative. It appears that restricting this plugin to non-
equality comparisons (<, >, <=, >=) and excluding equality comparisons (==, !=)
is a useful heuristic to avoid such false positives. The only remainging false
positive I found was 0288c8ffec "Rephrase cast
from sal_Int32 to sal_uInt32".
But which of course does not mean that there were no further false positivies
that I missed. So this commit may accidentally introduce some false hits of the
assert in o3tl::make_unsigned. At least, it passed a full (Linux ASan+UBSan
--enable-dbgutil) `make check && make screenshot`.
It is by design that o3tl::make_unsigned only accepts signed integer parameter
types (and is not defined as a nop for unsigned ones), to avoid unnecessary uses
which would in general be suspicious. But the STATIC_ARRAY_SELECT macro in
include/oox/helper/helper.hxx is used with both signed and unsigned types, so
needs a little oox::detail::make_unsigned helper function for now. (The
ultimate fix being to get rid of the macro in the first place.)
Change-Id: Ia4adc9f44c70ad1dfd608784cac39ee922c32175
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/87556
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
...about calling delete for a std::atomic<T*>. It ultimately turned out that I
didn't need such a std::atomic<T*> variable after all, but it can't hurt to have
this fix for loplugin:vclwidgets anyway.
Change-Id: I2ca058e9c39efa6c5386e6a320bed4bf8ab5f5d5
Reviewed-on: https://gerrit.libreoffice.org/85266
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
We already get -Wdeprecated-copy (warning about implicitly defined copy
functions that will in the future be deleted because other user-provided copy
functions exist) automatically through -Wextra, where available.
-Wdeprecated-copy-dtor (warning about implicitly defined copy functions that
will in the future be deleted because of a user-provided dtor) is split off
into its own warning excluded from -Wextra for somewhat unclear reasons, see the
discussion at <https://gcc.gnu.org/bugzilla/buglist.cgi?quicksearch=88136>
"-Wdeprecated-copy is draconian and shouldn't be in -Wall".
But -Wdeprecated-copy-dtor has been useful in finding issues (esp. the Clang 10
trunk version, which, unlike the GCC 9 version, also finds copy functions that
are implicitly defined because they are used from template instantiations), see
3e59716375 "Prevent
BroadcastRecalcOnRefMoveHandler copies" and
4f98cd0f9ce9c2a331a5d34b3ef9d18f9bb6b235 "ScShapeChild has broken copy
functions".
We need to disable -Wdeprecated-copy-dtor in files included from external/boost,
and in two compilerplugin/clang/test/ files.
Change-Id: I74b159c3a046e23661473ddbfe53c92c4136a9db
Reviewed-on: https://gerrit.libreoffice.org/85073
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
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
55e596956e "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 4f8a744c4f "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 314f15bff0 "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>
To mitigate the dangers of silently breaking ADL when moving enums into unnamed
namespaces (see the commit message of 206b5b2661
"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>
...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 7e09d08807 "Fix useless
assert(true) (which would never fire)" and
122a0be8ae "Fix useless assert(true) (which would
never fire)", plus 5f0d6df7f5 "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>
