If you run a recent daily build of LO (see build info below), create a new LO writer document, then quite, you get a console message saying: warn:unotools.config:4895:4895:unotools/source/config/configmgr.cxx:153: ConfigManager not empty Now, either it's a problem that the ConfigManager is not empty, in which case it should be properly emptied; or it's not a problem in which case the warning needs to be removed as console noise.
Build info: Version: 7.3.0.0.alpha0+ / LibreOffice Community Build ID: c998691e22ceda15c89d55cf7005201f0392dadb CPU threads: 4; OS: Linux 5.10; UI render: default; VCL: gtk3 Locale: en-US (en_IL); UI: en-US TinderBox: Linux-rpm_deb-x86_64@86-TDF, Branch:master, Time: 2021-10-14_11:54:20 Calc: threaded
On pc Debian x86-64 with master sources updated today, I could reproduce this.
Created attachment 175790 [details] bt with debug symbols for "Office.Commands/Execute" After adding some trace at registerConfigItem and removeConfigItem and when comparing, the only remnant was "Office.Commands/Execute". So I retrieved a bt from where "Office.Commands/Execute" is registered.
Stephan/Noel: In framework/source/services/frame.cxx I tried to change m_aCommandOptions from SvtCommandOptions to std::unique_ptr<SvtCommandOptions> but it failed with: #0 std::__shared_ptr<SvtCommandOptions_Impl, (__gnu_cxx::_Lock_policy)2>::get() const (this=0x20) at /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/shared_ptr_base.h:1330 #1 0x00007fbbb1490848 in std::__shared_ptr_access<SvtCommandOptions_Impl, (__gnu_cxx::_Lock_policy)2, false, false>::_M_get() const (this=0x20) at /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/shared_ptr_base.h:1027 #2 0x00007fbbb1485c35 in std::__shared_ptr_access<SvtCommandOptions_Impl, (__gnu_cxx::_Lock_policy)2, false, false>::operator->() const (this=0x20) at /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/shared_ptr_base.h:1021 #3 0x00007fbbb1484616 in SvtCommandOptions::EstablishFrameCallback(com::sun::star::uno::Reference<com::sun::star::frame::XFrame> const&) (this=0x0, xFrame= uno::Reference to ((anonymous namespace)::XFrameImpl *) 0x2cbd490) at unotools/source/config/cmdoptions.cxx:340 #4 0x00007fbbb61dc9b2 in (anonymous namespace)::XFrameImpl::initListeners() (this=0x2cbd420) at framework/source/services/frame.cxx:509 I noticed the use of weak reference and std::make_shared in unotools/source/config/cmdoptions.cxx. Any idea how to fix this part?
LibreOffice generates tons of false positive warnings, it is just one of those things that happens, no need to worry.
Noel: so does it mean we may remove the SAL_WARN_IF in: 152 utl::ConfigManager::~ConfigManager() { 153 SAL_WARN_IF(!items_.empty(), "unotools.config", "ConfigManager not empty"); 154 } to reduce console noise?
(In reply to Noel Grandin from comment #5) > LibreOffice generates tons of false positive warnings, It actually generates very few false warnings. At least the daily master branch build, anyway. > it is just one of those things that happens, no need to worry. Either this is a false warning, in which case it should not be made; or it is a valid warning, in which case the config manager should be emptied. Which is it?
I gave a try with https://gerrit.libreoffice.org/c/core/+/123762
Julien, I tested out your patch. It looks good. I respectfully disagree with the reviewer - a straight loop is fine for now. My only question is: why would "Office.Commands/Execute" be a problem? I have a utility that shows that "Setup/L10N" doesn't get cleared... Eval, if you want to improve upon the patch, why not fix it after the patch is committed. I can't see any reason why this is more or less readable than having to setup several lambdas for a custom built for_each_if() function! Which would be replaced with std::ranges in C++20.
Evan, seems to me that ConfigManager is never emptied. Doesn't seem like a false positive to me... Noel, why do you say it is a false positive?
From IRC: [06:02:55] <chris_wot> noelgrandin you wrote in https://bugs.documentfoundation.org/show_bug.cgi?id=145175 that "LibreOffice generates tons of false positive warnings, it is just one of those things that happens, no need to worry." [06:02:57] <IZBot> bug 145175: LibreOffice-Writer normal/medium NEW ConfigManager is not empty when quitting LO Writer (and LO) [06:04:08] <noelgrandin> exactly [06:04:29] <noelgrandin> why would we worry? it's not causing a problem [06:06:23] <chris_wot> so in that case, better to remove the warning? [06:06:44] <chris_wot> if we don't care about the fact that it isn't removed, why muddy up the output with the warning? [06:06:44] <noelgrandin> they can be useful, if you are chasing another related bug [06:07:03] <chris_wot> I'm confused, is it useful or not? [06:07:11] <chris_wot> have we had problems with config mgr? [06:07:30] <chris_wot> it's annoying to have it show output on the workben utilities we are creating [06:08:18] <chris_wot> noelgrandin does this mean it should be a WONTFIX then? [06:08:52] <noelgrandin> it would be a WONTFIX
(In reply to Chris Sherlock from comment #11) Which person has which nick and which function w.r.t. LO development? :-) Also... > [06:08:52] <noelgrandin> it would be a WONTFIX Already explained why that can't be right - whether one believes the config manager should be empty or not. And the IRC chat explained it too... > I respectfully disagree with the reviewer - a straight loop is fine for now. It's not _terrible_; it's just a question of style. I didn't say I reject the patch or anything. > Eyal, if you want to improve upon the patch, why not fix it after the patch is committed. Well, possibly because I'm not really an LO developer. There's only a small chance I would dive into checking out, building and testing. > I can't see any reason why this is more or less readable than having to setup several lambdas for a custom built for_each_if() function! Well, because it separates out the different semantic aspects of the code rather than weaving them together. It is idiomatic according to this paradigm: https://www.youtube.com/watch?v=W2tWOdzgXHA but obviously it is at least partially a matter of taste.