Armin Le Grand (allotropia) ae7807c889 ITEM: No longer register Items at Pool
The issue is that the flag RegisteredAtPool
at the SfxPoolItem is Pool-dependent: It marks that
the Item is registeres at *one* Pool/Model. This
makes it Pool-dependent. Due to this there is no way
to share Items that need to be registered globally/
in multiple Pools/Models what is one of the goals
for optimal sharing.

We can also not live without having access to all
Items associated with the Pool, due to mechanisms
in place like the Surrogate stuff.

This again is used for two purposes:

(1) Access all Items associated with one Pool/Model,
at least that is the assumption. This is not valid
since it gets corrupted with a single ItemSet/Holder
used that does not host model data, e.g. an open
Dialog or the Sidebar (or...). But works in
principle.

(2) Access extra-Items that are held nowhere and
are created using DirectPutItemInPool, e.g. infos
for a Dialog. These would need a instance/place to
host them, the Pool is (ab)used for that.

Both are 'compromizes' (to not use a more bad word)
and should not exist. (1) should iterate over the
Model and do actions. There are even places that
use (1) to *change* Items, by casting them to
non-const, even RefCounted ones, so having no control
over what all might be changed doing so. Since we
talk about ca. 100+ places there is no way to get
away from this - I can imagine how this happened:
The 'poolable' attr traditionally needed for the old
binary format was one day 'used' to not need to
iterate over the Model, an API was added to access
and this usage was copied. Sigh..

It is even used when ODF is loaded: E.g. the
FillStyle is imported from XML, interpreted, and
put into an ItemSet. Then it gets set at the
XShape using UNO API and a *name* -> that name and
the Surrogate mechanism is used to find and set the
FillStyle at the Model Object. The FillStyle could
probably just be set using UNO API and the data
directly.

The association between Model/Pool and Item is
created by the object hosting the Item, these are
ItemSets and ItemHolders. Thus it is possible to
register these at the Pool. This allows to iterate
and collect the Items associated with the Pool
and keep the Surrogate mechanism alive.
This is the main change done here. It limits
the registrations to Items for which (at the Pool)
the NeedsPoolRegistration is set, also
Item-independent. Speed is okay, I saw no big
changes in my tests here. The registration is
just pointers, no ownership or RefCounting needed
here.

The advantage is that Items get closer to be
shared office-wide, they can be referenced by
multiple ItemSets (RefCnt) associated with
different Pools/Models.
NOTE: This is not true for SfxSetItems, these are
and will stay Pool-dependent due to their need
to a Pool in the contained ItemSet.
Note that we have ca. six deivations of SfxSetItem,
but ca. 500+ Item derivations, so not too bad.

For the usages of Surrogates to change existing,
RefCounted Items: These can now at least be
changed - if they show up to be problematic - to
iterate over the registered ItemSets and change
Items there the correct way: Set a changed one
at the ItemSet. That also allows Objects to
*react* on ItemChanges, there is no way to do
that with the existing 'compromize'...

UnitTests show that this already works well for
SC and SD, but SW has still some issues. I will
put this to gerrit now, but there will be
additional work.

A involved problem is the current DefaultItem
handling and the state the Pool implementation
is in. E.g. StaticDefaults are not really static,
Pools hard-delete the DefaultItems (forcing the
RefCnt to zero to not have the destructor
complain) and other quirks. Looking at that
right now, hoping to get this change done without
having to change that too much.

I thought about adapting PoolItemTest to this,
but it is only related to DirectPutItemInPool
which is mostly gone and hopefully completely
soon.
Nonetheless I adapted that mechanism to use a
list of SfxPoolItemHolder at the Pool. That makes
it safe and abandons the need for indirect
garbage collection removal at the Pool.

Change-Id: Ib47f21dafa989202930919eace5f7e9c5632ce96
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161896
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>
2024-01-12 12:15:54 +01:00

213 lines
7.8 KiB
C++

/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
/*
* This file is part of the LibreOffice project.
*
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/.
*/
#include <cassert>
#include <string>
#include <iostream>
#include <unordered_map>
#include <unordered_set>
#include "plugin.hxx"
#include "check.hxx"
#include "config_clang.h"
#include "clang/AST/CXXInheritance.h"
#include "clang/AST/StmtVisitor.h"
// Look for variables that either
// (a) could be statically initialised, without runtime code, and warn
// (b) variables that are statically declared, but require runtime initialisation, and warn
//
// e.g.
// static const OUString[] XXX { "xxx" };
// requires runtime initialisation, so should rather be declared as OUStringLiteral
// and
// static int[] XXX { 1,2 };
// can be declared const since it does not require runtime initialisation.
namespace
{
class StaticVar : public loplugin::FilteringPlugin<StaticVar>
{
public:
explicit StaticVar(loplugin::InstantiationData const& data)
: FilteringPlugin(data)
{
}
virtual void run() override
{
std::string fn(handler.getMainFileName());
loplugin::normalizeDotDotInFilePath(fn);
if (
// uses icu::UnicodeString
fn == SRCDIR "/l10ntools/source/xmlparse.cxx"
// contains mutable state
|| fn == SRCDIR "/sal/osl/unx/signal.cxx"
|| fn == SRCDIR "/sal/qa/rtl/digest/rtl_digest.cxx"
|| fn == SRCDIR "/sal/qa/rtl/strings/test_oustring_endswith.cxx"
|| fn == SRCDIR "/sal/qa/rtl/strings/test_oustring_convert.cxx"
// contains mutable state
|| fn == SRCDIR "/vcl/unx/generic/dtrans/X11_selection.cxx"
|| fn == SRCDIR "/sax/qa/cppunit/xmlimport.cxx"
|| fn == SRCDIR "/pyuno/source/module/pyuno.cxx"
|| fn == SRCDIR "/pyuno/source/module/pyuno_module.cxx"
|| fn == SRCDIR "/pyuno/source/module/pyuno_struct.cxx"
// TODO for this one we need a static OUString
|| fn == SRCDIR "/xmloff/source/core/xmltoken.cxx"
// mutable
|| fn == SRCDIR "/basic/source/runtime/stdobj.cxx"
// TODO this needs more extensive cleanup
|| fn == SRCDIR "/connectivity/source/drivers/postgresql/pq_statics.cxx"
// mutable
|| fn == SRCDIR "/hwpfilter/source/hwpreader.cxx"
// mutable
|| fn == SRCDIR "/sw/source/filter/basflt/fltini.cxx"
// mutable
|| fn == SRCDIR "/sw/source/uibase/docvw/srcedtw.cxx"
// mutable
|| fn == SRCDIR "/forms/source/misc/limitedformats.cxx"
// aHTMLOptionTab is ordered by useful grouping, so let it sort at runtime
|| fn == SRCDIR "/svtools/source/svhtml/htmlkywd.cxx"
// TODO sorting some of these tables will be a lot of work...
|| fn == SRCDIR "/sw/source/filter/ww8/ww8par6.cxx"
// this only triggers on older versions of clang, not sure why
// in any case, it is actually about the array in vcl/inc/units.hrc, which we can't change
|| fn == SRCDIR "/vcl/source/app/svdata.cxx"
// I tried doing this, but got very weird unit test failures, apparently sorting this table
// disturbs some code elsewhere
|| fn == SRCDIR "/svx/source/unodraw/unoprov.cxx"
// aRTFTokenTab is ordered by useful grouping, so let it sort at runtime
|| fn == SRCDIR "/svtools/source/svrtf/rtfkeywd.cxx")
return;
TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
}
bool VisitVarDecl(VarDecl const*);
};
static bool containsNonLiteral(Expr const* expr)
{
expr = expr->IgnoreImplicit();
if (auto initList = dyn_cast<InitListExpr>(expr))
{
for (unsigned i = 0; i < initList->getNumInits(); ++i)
if (containsNonLiteral(initList->getInit(i)))
return true;
}
else if (auto constructExpr = dyn_cast<CXXConstructExpr>(expr))
{
for (Expr const* arg : constructExpr->arguments())
if (containsNonLiteral(arg))
return true;
}
else if (isa<MemberExpr>(expr))
return true;
else if (auto declRefExpr = dyn_cast<DeclRefExpr>(expr))
{
auto varDecl = dyn_cast_or_null<VarDecl>(declRefExpr->getDecl());
return varDecl && varDecl->isLocalVarDeclOrParm();
}
else if (isa<CXXMemberCallExpr>(expr))
return true;
else if (auto castExpr = dyn_cast<CXXFunctionalCastExpr>(expr))
return containsNonLiteral(castExpr->getSubExpr());
else if (auto unaryOp = dyn_cast<UnaryOperator>(expr))
return containsNonLiteral(unaryOp->getSubExpr());
return false;
}
bool StaticVar::VisitVarDecl(VarDecl const* varDecl)
{
if (ignoreLocation(varDecl))
return true;
if (!varDecl->hasInit())
return true;
auto initList = dyn_cast_or_null<InitListExpr>(varDecl->getInit());
if (!initList)
return true;
if (varDecl->isExceptionVariable() || isa<ParmVarDecl>(varDecl))
return true;
if (!varDecl->getType()->isArrayType())
return true;
auto elementType = varDecl->getType()->getBaseElementTypeUnsafe();
if (!elementType->isRecordType())
return true;
auto elementRecordDecl
= dyn_cast_or_null<CXXRecordDecl>(elementType->getAs<RecordType>()->getDecl());
if (!elementRecordDecl)
return true;
if (containsNonLiteral(initList))
return true;
if (elementRecordDecl->hasTrivialDestructor())
{
if (varDecl->isLocalVarDecl())
{
if (varDecl->getStorageDuration() == SD_Static && varDecl->getType().isConstQualified())
return true;
}
else
{
if (varDecl->getType().isConstQualified())
return true;
}
// TODO cannot figure out how to make the loplugin::TypeCheck stuff match this
// std::string typeName = varDecl->getType().getAsString();
// if (typeName == "std::va_list" || typeName == "va_list")
// return true;
auto const tcElement = loplugin::TypeCheck(elementRecordDecl);
if (tcElement.Struct("ContextID_Index_Pair").GlobalNamespace())
return true;
if (tcElement.Class("SfxSlot").GlobalNamespace())
return true;
if (varDecl->isLocalVarDecl())
report(DiagnosticsEngine::Warning, "var should be static const, or allowlisted",
varDecl->getLocation())
<< varDecl->getSourceRange();
else
report(DiagnosticsEngine::Warning, "var should be const, or allowlisted",
varDecl->getLocation())
<< varDecl->getSourceRange();
}
else
{
if (varDecl->isLocalVarDecl())
{
if (varDecl->getStorageDuration() != SD_Static
|| !varDecl->getType().isConstQualified())
return true;
}
else
{
if (!varDecl->getType().isConstQualified())
return true;
}
if (varDecl->isLocalVarDecl())
report(DiagnosticsEngine::Warning, "static const var requires runtime initialization?",
varDecl->getLocation())
<< varDecl->getSourceRange();
else
report(DiagnosticsEngine::Warning, "static var requires runtime initialization?",
varDecl->getLocation())
<< varDecl->getSourceRange();
}
return true;
}
loplugin::Plugin::Registration<StaticVar> X("staticvar", false);
}
/* vim:set shiftwidth=4 softtabstop=4 expandtab: */