[squid-dev] [PATCH] YesNoNone class updates
Amos Jeffries
squid3 at treenet.co.nz
Wed Jan 13 22:56:41 UTC 2016
On 13/01/2016 5:11 a.m., Alex Rousskov wrote:
> On 01/11/2016 06:36 PM, Amos Jeffries wrote:
>> Update class YesNoNone to use C++11 features which allow better boolean
>> conversion, assignment, copy, and move ctors.
>
> The safer boolean operator is welcomed, of course.
>
>
>> + YesNoNone &operator =(bool beSet) {configure(beSet); return *this;}
>
> This is a bad idea IMO. If we want to distinguish configuration-set
> options from code-set options, we should not have implicit conversions
> like that. The caller should use the existing configure() method instead.
>
The impicit indicator is intended for marking default values of config.
We code those as initializer lists. Thus the ctor is implicit.
Explicit assignment of different boolean values happens when
ConfigParser finds some other value explicitly configured. Thus the
assign is configure()'d.
I've now made the assignment operator deleted/forbidden and documented
the two use cases for setting values instead of it.
>
>> + explicit YesNoNone(const YesNoNone &) = default;
>> + YesNoNone &operator =(const YesNoNone &) = default;
>> +
>> + explicit YesNoNone(YesNoNone &&) = default;
>> + YesNoNone &operator =(YesNoNone &&) = default;
>> +
>> + ~YesNoNone() = default;
>
> All the added "= default" methods should be removed as well AFAICT. We
> are not violating Big Three or Big Five rules without them, and adding
> them misleads the reader into thinking there is something complex about
> this class internal data structures and memory usage.
>
Removed again.
FWIW; these were added to resolve compile errors earlier with the
ServerOptions clear() function requiring a implicitly-deleted assignment
operator.
>
>> Distinction is made between implicit and explicit configuration.
>
> Why do we need this increased complexity? Your patch does not seem to
> show any places where this will be used. I am not saying this added
> complexity is necessarily wrong, but it is missing the motivation/usage
> examples to justify it.
This was written in context of using it to complete the final audit
request for the bug 4005 patch instead of multiple bools. Which will be
applied after this passes audit.
The PeerConfig/ServerConfig need initialization lists for the config
defaults to prevent forcing users to manually configure each flag to its
default value.
If the configured() method is used by the ctor it will act exactly like
manual configuration - producing assert() if the user ever dares to
configure the option to a non-default value.
Explicit assignment of different boolean values seems reasonable when
ConfigParser finds some other value explicitly in squid.conf.
Thus the assign is better suited to be configure()'d than implicit.
I managed to get around this oddity though by simply forbidding
bool-assignment. Instead require the bool-ctor to be used to create an
implicit default config option then copy-assign it into place.
You can see an example in this patches updates to Christos recent patch.
>
>> + /// \return true iff enabled; asserts if the option has not been configured
>> + explicit operator bool() const;
>> +
>> + /// \return true iff disabled; asserts if the option has not been configured
>> + bool operator !() const;
>
>
> The above descriptions became very misleading after your changes. The
> [old] code is still correct -- it asserts on unspecified option values.
> However, specifying the value no longer requires calling configure()
> because you have added "implicit" configuration via constructor.
> s/configured/specified/
Re-written.
>
>> + explicit YesNoNone(bool beSet);
>
> Please add a description to say that this constructor specifies the
> option value implicitly.
>
>
>> +bool
>> +YesNoNone::operator !() const
>> +{
>> + assert(option != YesNoNone::optUnspecified);
>> + return option == YesNoNone::optDisabled;
>> }
>
> Please never reverse the existing operator bool code. Inline the
> negation of that operator call instead.
These were trilean tests, not bool. boolean-inverting a trilean to
produce one of the other states violates the nature of the third state.
ie. the assert happening correctly when !X is invalid.
I have removed the !() operator entirely now. With the setHow change you
proposed the trilean became boolean again and the compiler can now
correctly infer the !() value by using implicit bool() conversion.
All other points are done as requested.
If there are no objections I will apply this patch and the bug 4005 fix
that depends on it tomorrow.
Amos
-------------- next part --------------
=== modified file 'src/Makefile.am'
--- src/Makefile.am 2016-01-05 13:00:50 +0000
+++ src/Makefile.am 2016-01-13 06:15:50 +0000
@@ -230,8 +230,6 @@
AsyncEngine.h \
cache_cf.h \
AuthReg.h \
- YesNoNone.h \
- YesNoNone.cc \
RefreshPattern.h \
cache_cf.cc \
CacheDigest.h \
@@ -920,7 +918,8 @@
tests/testSBufList \
tests/testConfigParser \
tests/testStatHist \
- tests/testLookupTable
+ tests/testLookupTable \
+ tests/testYesNoNone
if HAVE_FS_ROCK
check_PROGRAMS += tests/testRock
@@ -1009,7 +1008,6 @@
log/access_log.h \
tests/stub_access_log.cc \
cache_cf.h \
- YesNoNone.h \
tests/stub_cache_cf.cc \
tests/stub_cache_manager.cc \
tests/stub_comm.cc \
@@ -1131,7 +1129,6 @@
log/access_log.h \
tests/stub_access_log.cc \
cache_cf.h \
- YesNoNone.h \
tests/stub_cache_cf.cc \
tests/stub_client_side.cc \
tests/stub_debug.cc \
@@ -1265,8 +1262,6 @@
cache_manager.cc \
cache_cf.h \
AuthReg.h \
- YesNoNone.h \
- YesNoNone.cc \
RefreshPattern.h \
cache_cf.cc \
CachePeer.cc \
@@ -1582,7 +1577,6 @@
tests/stub_access_log.cc \
tests/stub_acl.cc \
cache_cf.h \
- YesNoNone.h \
tests/stub_cache_cf.cc \
tests/stub_cache_manager.cc \
tests/stub_client_db.cc \
@@ -1698,8 +1692,6 @@
tests/stub_CacheDigest.cc \
cache_cf.h \
AuthReg.h \
- YesNoNone.h \
- YesNoNone.cc \
RefreshPattern.h \
cache_cf.cc \
CachePeer.cc \
@@ -1947,8 +1939,6 @@
cache_manager.cc \
cache_cf.h \
AuthReg.h \
- YesNoNone.h \
- YesNoNone.cc \
RefreshPattern.h \
cache_cf.cc \
CachePeer.cc \
@@ -2190,8 +2180,6 @@
BodyPipe.cc \
cache_cf.h \
AuthReg.h \
- YesNoNone.h \
- YesNoNone.cc \
RefreshPattern.h \
cache_cf.cc \
CachePeer.cc \
@@ -2460,7 +2448,6 @@
mime_header.h \
String.cc \
cache_cf.h \
- YesNoNone.h \
$(SBUF_SOURCE) \
SBufAlgos.h \
SBufAlgos.cc \
@@ -2526,8 +2513,6 @@
cache_manager.cc \
cache_cf.h \
AuthReg.h \
- YesNoNone.h \
- YesNoNone.cc \
RefreshPattern.h \
cache_cf.cc \
debug.cc \
@@ -2867,7 +2852,6 @@
tests/stub_access_log.cc \
tests/stub_acl.cc \
cache_cf.h \
- YesNoNone.h \
tests/stub_cache_cf.cc \
tests/stub_cache_manager.cc \
tests/stub_client_side_request.cc \
@@ -2973,7 +2957,6 @@
tests/testString.cc \
tests/testString.h \
cache_cf.h \
- YesNoNone.h \
tests/stub_cache_cf.cc \
tests/stub_cache_manager.cc \
tests/stub_cbdata.cc \
@@ -3094,7 +3077,6 @@
ConfigOption.cc \
tests/stub_acl.cc \
cache_cf.h \
- YesNoNone.h \
tests/stub_cache_cf.cc \
tests/stub_helper.cc \
cbdata.cc \
@@ -3294,7 +3276,6 @@
log/access_log.h \
tests/stub_access_log.cc \
cache_cf.h \
- YesNoNone.h \
tests/stub_cache_cf.cc \
client_db.h \
tests/stub_cache_manager.cc \
@@ -3377,8 +3358,6 @@
BodyPipe.cc \
cache_cf.h \
AuthReg.h \
- YesNoNone.h \
- YesNoNone.cc \
RefreshPattern.h \
cache_cf.cc \
tests/stub_cache_manager.cc \
@@ -3719,7 +3698,6 @@
tests/testConfigParser.cc \
tests/testConfigParser.h \
cache_cf.h \
- YesNoNone.h \
tests/stub_cache_cf.cc \
tests/stub_cache_manager.cc \
tests/stub_cbdata.cc \
@@ -3825,6 +3803,18 @@
$(XTRA_LIBS)
tests_testEnumIterator_DEPENDENCIES =
+tests_testYesNoNone_SOURCES = \
+ tests/testYesNoNone.cc \
+ tests/testYesNoNone.h
+nodist_tests_testYesNoNone_SOURCES = \
+ tests/STUB.h \
+ base/YesNoNone.h
+tests_testYesNoNone_LDADD= \
+ $(SQUID_CPPUNIT_LIBS) \
+ $(COMPAT_LIB) \
+ $(XTRA_LIBS)
+tests_testYesNoNone_LDFLAGS = $(LIBADD_DL)
+
TESTS += testHeaders
## Special Universal .h dependency test script
=== modified file 'src/SquidConfig.h'
--- src/SquidConfig.h 2016-01-01 00:12:18 +0000
+++ src/SquidConfig.h 2016-01-13 06:15:33 +0000
@@ -11,6 +11,7 @@
#include "acl/forward.h"
#include "base/RefCount.h"
+#include "base/YesNoNone.h"
#include "ClientDelayConfig.h"
#include "DelayConfig.h"
#include "helper/ChildConfig.h"
@@ -23,7 +24,6 @@
#include "ssl/support.h"
#endif
#include "store/forward.h"
-#include "YesNoNone.h"
#if USE_OPENSSL
class sslproxy_cert_sign;
=== removed file 'src/YesNoNone.cc'
--- src/YesNoNone.cc 2016-01-01 00:12:18 +0000
+++ src/YesNoNone.cc 1970-01-01 00:00:00 +0000
@@ -1,23 +0,0 @@
-/*
- * Copyright (C) 1996-2016 The Squid Software Foundation and contributors
- *
- * Squid software is distributed under GPLv2+ license and includes
- * contributions from numerous individuals and organizations.
- * Please see the COPYING and CONTRIBUTORS files for details.
- */
-
-#include "squid.h"
-#include "YesNoNone.h"
-
-YesNoNone::operator void*() const
-{
- assert(option != 0); // must call configure() first
- return option > 0 ? (void*)this : NULL;
-}
-
-void
-YesNoNone::configure(bool beSet)
-{
- option = beSet ? +1 : -1;
-}
-
=== modified file 'src/adaptation/ServiceConfig.h'
--- src/adaptation/ServiceConfig.h 2016-01-13 10:10:20 +0000
+++ src/adaptation/ServiceConfig.h 2016-01-13 17:06:05 +0000
@@ -11,9 +11,9 @@
#include "adaptation/Elements.h"
#include "base/RefCount.h"
+#include "base/YesNoNone.h"
#include "security/PeerOptions.h"
#include "SquidString.h"
-#include "YesNoNone.h"
namespace Adaptation
{
=== modified file 'src/adaptation/ecap/ServiceRep.cc'
--- src/adaptation/ecap/ServiceRep.cc 2016-01-13 10:10:20 +0000
+++ src/adaptation/ecap/ServiceRep.cc 2016-01-13 22:15:24 +0000
@@ -171,7 +171,7 @@
{
Adaptation::Service::finalize();
if (!cfg().connectionEncryption.configured())
- writeableCfg().connectionEncryption.configure(true);
+ writeableCfg().connectionEncryption = YesNoNone(true);
theService = FindAdapterService(cfg().uri);
if (theService) {
try {
=== modified file 'src/adaptation/icap/ServiceRep.cc'
--- src/adaptation/icap/ServiceRep.cc 2016-01-13 12:12:11 +0000
+++ src/adaptation/icap/ServiceRep.cc 2016-01-13 18:38:23 +0000
@@ -85,10 +85,10 @@
if (cfg().secure.encryptTransport) {
debugs(3, DBG_IMPORTANT, "Initializing service " << cfg().resource << " SSL context");
sslContext = writeableCfg().secure.createClientContext(true);
- if (!cfg().connectionEncryption.configured())
- writeableCfg().connectionEncryption.configure(true);
- } else if (!cfg().connectionEncryption.configured())
- writeableCfg().connectionEncryption.configure(false);
+ }
+
+ if (!cfg().connectionEncryption.configured())
+ writeableCfg().connectionEncryption = YesNoNone(cfg().secure.encryptTransport);
theSessionFailures.configure(TheConfig.oldest_service_failure > 0 ?
TheConfig.oldest_service_failure : -1);
=== modified file 'src/base/Makefile.am'
--- src/base/Makefile.am 2016-01-01 00:12:18 +0000
+++ src/base/Makefile.am 2016-01-13 06:16:06 +0000
@@ -38,4 +38,5 @@
Subscription.h \
TextException.cc \
TextException.h \
- TidyPointer.h
+ TidyPointer.h \
+ YesNoNone.h
=== renamed file 'src/YesNoNone.h' => 'src/base/YesNoNone.h'
--- src/YesNoNone.h 2016-01-01 00:12:18 +0000
+++ src/base/YesNoNone.h 2016-01-13 22:13:56 +0000
@@ -9,27 +9,59 @@
#ifndef SQUID_YESNONONE_H_
#define SQUID_YESNONONE_H_
-/// Used for boolean enabled/disabled options with complex default logic.
-/// Allows Squid to compute the right default after configuration.
-/// Checks that not-yet-defined option values are not used.
+// TODO: generalize / template to non-boolean option types
+// and make YesNoNone the boolean instance of the template
+
+/**
+ * Used for boolean enabled/disabled options with complex default logic.
+ * Allows Squid to compute the right default after configuration.
+ * Checks that not-yet-defined option values are not used.
+ * Allows for implicit default Yes/No values to be used by initialization
+ * without configure() being called, but not dumped as squid.conf content.
+ */
class YesNoNone
{
-// TODO: generalize to non-boolean option types
+ enum SetWith : uint8_t { optUnspecified = 0, optImplicit = 1, optConfigured = 2 };
+
public:
- YesNoNone(): option(0) {}
-
- /// returns true iff enabled; asserts if the option has not been configured
- operator void *() const; // TODO: use a fancy/safer version of the operator
-
- /// enables or disables the option;
- void configure(bool beSet);
-
- /// whether the option was enabled or disabled, by user or Squid
- bool configured() const { return option != 0; }
+ // this constructor initializes to 'unspecified' state
+ YesNoNone():
+ option(false),
+ setWith(optUnspecified)
+ {}
+
+ // this constructor initializes to 'implicit' state
+ explicit YesNoNone(bool beSet):
+ option(beSet),
+ setWith(optImplicit)
+ {}
+
+ /** forbidden operation.
+ * Use x.configure(bool) when the value is configured.
+ * Use x = YesNoNone(bool) to assign defaults.
+ */
+ YesNoNone &operator =(bool) = delete;
+
+ /// the boolean equivalent of the value stored.
+ /// asserts if the value has not been set.
+ explicit operator bool() const {
+ assert(setWith != optUnspecified);
+ return option;
+ }
+
+ /// enables or disables the option; updating to 'configured' state
+ void configure(bool beSet) {
+ option = beSet;
+ setWith = optConfigured;
+ }
+
+ /// whether the option was enabled or disabled,
+ /// by squid.conf values resulting in explicit configure() usage.
+ bool configured() const {return setWith == optConfigured;}
private:
- enum { optUnspecified = -1, optDisabled = 0, optEnabled = 1 };
- int option; ///< configured value or zero
+ bool option; ///< configured value or false
+ uint8_t setWith; ///< initialized instead of configured
};
#endif /* SQUID_YESNONONE_H_ */
=== modified file 'src/tests/stub_cache_cf.cc'
--- src/tests/stub_cache_cf.cc 2016-01-01 00:12:18 +0000
+++ src/tests/stub_cache_cf.cc 2016-01-13 22:14:44 +0000
@@ -12,7 +12,6 @@
#include "acl/Acl.h"
#include "ConfigParser.h"
#include "wordlist.h"
-#include "YesNoNone.h"
#define STUB_API "cache_cf.cc"
#include "tests/STUB.h"
@@ -28,5 +27,4 @@
void ConfigParser::ParseUShort(unsigned short *var) STUB
void dump_acl_access(StoreEntry * entry, const char *name, acl_access * head) STUB
void dump_acl_list(StoreEntry*, ACLList*) STUB
-YesNoNone::operator void*() const { STUB_NOP; return NULL; }
=== modified file 'src/tests/testRock.cc'
--- src/tests/testRock.cc 2016-01-01 00:12:18 +0000
+++ src/tests/testRock.cc 2016-01-13 21:30:30 +0000
@@ -57,6 +57,8 @@
if (0 > system ("rm -rf " TESTDIR))
throw std::runtime_error("Failed to clean test work directory");
+ Config.memShared = YesNoNone(false);
+
// use current directory for shared segments (on path-based OSes)
Ipc::Mem::Segment::BasePath = getcwd(cwd,MAXPATHLEN);
if (Ipc::Mem::Segment::BasePath == NULL)
=== modified file 'src/tests/testStoreController.cc'
--- src/tests/testStoreController.cc 2016-01-01 00:12:18 +0000
+++ src/tests/testStoreController.cc 2016-01-13 21:36:10 +0000
@@ -55,6 +55,8 @@
if (inited)
return;
+ Config.memShared = YesNoNone(false);
+
Mem::Init();
Config.Store.avgObjectSize = 1024;
=== modified file 'src/tests/testUfs.cc'
--- src/tests/testUfs.cc 2016-01-01 00:12:18 +0000
+++ src/tests/testUfs.cc 2016-01-13 21:40:02 +0000
@@ -65,6 +65,8 @@
Config.replPolicy = new RemovalPolicySettings;
Config.replPolicy->type = xstrdup("lru");
+ Config.memShared = YesNoNone(false);
+
/* garh garh */
storeReplAdd("lru", createRemovalPolicy_lru);
=== added file 'src/tests/testYesNoNone.cc'
--- src/tests/testYesNoNone.cc 1970-01-01 00:00:00 +0000
+++ src/tests/testYesNoNone.cc 2016-01-13 06:15:18 +0000
@@ -0,0 +1,56 @@
+/*
+ * Copyright (C) 1996-2016 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+#include "squid.h"
+#include "base/YesNoNone.h"
+#include "tests/testYesNoNone.h"
+#include "unitTestMain.h"
+
+#include <stdexcept>
+
+CPPUNIT_TEST_SUITE_REGISTRATION( testYesNoNone );
+
+void
+testYesNoNone::testBasics()
+{
+ // unconfigured, non-implicit
+ {
+ YesNoNone v;
+ CPPUNIT_ASSERT_EQUAL(false, v.configured());
+ // cannot test the value it is 'undefined' and will assert
+ }
+ // implicit dtor test
+
+ // unconfigured, implicit true
+ {
+ YesNoNone v(true);
+ CPPUNIT_ASSERT_EQUAL(false, v.configured());
+ CPPUNIT_ASSERT(v);
+ CPPUNIT_ASSERT_EQUAL(true, static_cast<bool>(v));
+
+ // check explicit setter method
+ v.configure(false);
+ CPPUNIT_ASSERT_EQUAL(true, v.configured());
+ CPPUNIT_ASSERT(!v);
+ CPPUNIT_ASSERT_EQUAL(false, static_cast<bool>(v));
+ }
+
+ // unconfigured, implicit false
+ {
+ YesNoNone v(false);
+ CPPUNIT_ASSERT_EQUAL(false, v.configured());
+ CPPUNIT_ASSERT(!v);
+ CPPUNIT_ASSERT_EQUAL(false, static_cast<bool>(v));
+
+ // check assignment operator
+ v = YesNoNone(true);
+ CPPUNIT_ASSERT_EQUAL(false, v.configured());
+ CPPUNIT_ASSERT(v);
+ CPPUNIT_ASSERT_EQUAL(true, static_cast<bool>(v));
+ }
+}
=== added file 'src/tests/testYesNoNone.h'
--- src/tests/testYesNoNone.h 1970-01-01 00:00:00 +0000
+++ src/tests/testYesNoNone.h 2016-01-11 10:48:11 +0000
@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) 1996-2016 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+#ifndef SQUID_SRC_TESTS_TESTYESNONONE_H
+#define SQUID_SRC_TESTS_TESTYESNONONE_H
+
+#include <cppunit/extensions/HelperMacros.h>
+
+/*
+ * demonstration test file, as new idioms are made they will
+ * be shown in the testYesNoNone source.
+ */
+
+class testYesNoNone : public CPPUNIT_NS::TestFixture
+{
+ CPPUNIT_TEST_SUITE( testYesNoNone );
+ /* note the statement here and then the actual prototype below */
+ CPPUNIT_TEST( testBasics );
+ CPPUNIT_TEST_SUITE_END();
+
+public:
+
+protected:
+ void testBasics();
+};
+
+#endif /* SQUID_SRC_TESTS_TESTYESNONONE_H */
+
More information about the squid-dev
mailing list