Contribute to Open Source. Search issue labels to find the right project for you!

Mentor: Greg Wiley

cjkoepke/code-mentors

Who Are You

I’m a user experience strategist, front-end developer and product manager. I occasionally tinker with smart home products. I’ve worked with companies in the Fortune 200, and helped build apps with 1M+ users. I love to teach and point to my sources so folks can learn more. I’m interested in helping a curious mind dig deeper into web and product development.

Oh, and Star Trek is better than Star Wars 😏🤓

Ways to Connect

Updated 25/06/2017 15:13 8 Comments

GSL tests broken with GCC 7.1.0

Microsoft/GSL
COLLECT_LTO_WRAPPER=/mnt/fscratch0/pub/build/root-gcc-7.1.0-system/libexec/gcc/x86_64-pc-linux-gnu/7.1.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ../../dependencies/gcc-7.1.0/configure --prefix=/mnt/fscratch0/pub/build/root-gcc-7.1.0-system --enable-languages=c,c++ --without-multilib
Thread model: posix
gcc version 7.1.0 (GCC)

This produces a ton of sign error spam when compiling tests.

Example:

                 from /mnt/devdiskvault/gfurnish/ap/GSL/tests/algorithm_tests.cpp:19:
/mnt/devdiskvault/gfurnish/ap/GSL/include/gsl/span: In constructor ‘constexpr gsl::span<ElementType, Extent>::span(gsl::span<ElementType, Extent>::element_type (&)[N])’:
/mnt/devdiskvault/gfurnish/ap/GSL/include/gsl/span:373:51: error: conversion to ‘long int’ from ‘long unsigned int’ may change the sign of the result [-Werror=sign-conversion]
         : storage_(&arr[0], details::extent_type<N>())

See log I haven’t looked into why this is happening yet but removing the -Wsign-conversion warning from tests does fix the issue.

Updated 12/06/2017 12:46 6 Comments

not_null comparison

Microsoft/GSL

This code causes compiler error C2678 in Visual Studio 2017. Why is that?

void test()
{
    struct A {};
    struct B : public A {} b;

    const B* pB{ &b }; 
    gsl::not_null<A*> pA{ &b };

    assert(pA == pB);
}
Updated 02/06/2017 05:00

Why are std::function objects wrapped in unique_ptr?

mikebmcl/P0267_RefImpl

Hi Mike,

I observed that several function objects (callbacks) are indirectly wrapped in unique pointers. Why?

e.g. ::std::unique_ptr<::std::function<void(display_surface& sfc)>> Draw_fn; ::std::unique_ptr<::std::function<void(display_surface& sfc)>> Size_change_fn; ::std::unique_ptr<::std::function<::std::experimental::io2d::rectangle(const display_surface&, bool&)>> _User_scaling_fn;

a std::function object can be empty and manages its memory allocation accordingly, because it implements type erasure. There is no need for an additional indirection. A callback that is not set still is checkable against nullptr. and checking the unique_ptr against nullptr is not enough, since the wrapped function object still could be empty.

Sorry for being terse, but I try to make it work on my mac (using xlib) and I am chasing why my input key events do not work.

Regards Peter.

Updated 10/05/2017 14:15 2 Comments

Update to Catch instead of UnitTest-cpp

Microsoft/GSL

I have an updated Travis CI script that adds Clang Format, Clang 3.9, Clang 4.0, and Git Check. Apparently, Clang 3.9 and Clang 4.0 have been broken for some time now as UnitTest-cpp segfaults with these compilers on it’s own unit tests.

To add to the issues with UnitTest-cpp, on my own project, UnitTest-cpp cannot compile at all on Cygwin, which means neither can the GSL which prevents automated testing with the GSL on AppVeyor Cygwin builds. We currently patch the GSL to remove UnitTest-cpp so that the GSL works in all environments (GCC, Clang, Cygwin and Windows).

The simple solution is to move to Catch.hpp which is a simple, header only library that works in all of these environments. Furthermore, the syntax is very similar to UnitTest-cpp, so the diff will not be that large. Catch.hpp is also a much larger project with regular updates, while UnitTest-cpp hasn’t been updated in 3 months.

My Proposal: - PR#1: Move to Catch.cpp, update unit tests - PR#2: Update Travis CI script

I already have PR#2 done, but I don’t want to start PR#1 without a blessing from @neilmacintosh

Updated 26/04/2017 00:19 10 Comments

Make install will also install UnitTest++

Microsoft/GSL

Currently, if you run make install it will also install UnitTest++. AFAIU every install command in a cmake file is run during install. There is no way to select which one to install. Since GSL adds UnitTest++ through a subfolder and UnitTest++ has an install command it will also get installed.

I will try to patch the UnitTest++ cmake file to avoid this.

[ 32%] Built target UnitTest++
[ 35%] Built target algorithm_tests
[ 38%] Built target multi_span_tests
[ 41%] Built target span_tests
[ 44%] Built target strided_span_tests
[ 47%] Built target string_span_tests
[ 50%] Built target at_tests
[ 52%] Built target bounds_tests
[ 55%] Built target byte_tests
[ 58%] Built target owner_tests
[ 61%] Built target notnull_tests
[ 64%] Built target assertion_tests
[ 67%] Built target utils_tests
[100%] Built target TestUnitTest++
Install the project...
-- Install configuration: ""
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/gsl
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/gsl/gsl
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/gsl/gsl_algorithm
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/gsl/gsl_assert
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/gsl/gsl_byte
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/gsl/gsl_util
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/gsl/multi_span
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/gsl/span
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/gsl/string_span
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/lib/libUnitTest++.a
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/AssertException.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/CheckMacros.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/Checks.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/CompositeTestReporter.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/Config.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/CurrentTest.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/DeferredTestReporter.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/DeferredTestResult.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/ExceptionMacros.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/ExecuteTest.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/HelperMacros.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/MemoryOutStream.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/ReportAssert.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/ReportAssertImpl.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/RequireMacros.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/RequiredCheckException.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/RequiredCheckTestReporter.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/Test.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/TestDetails.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/TestList.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/TestMacros.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/TestReporter.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/TestReporterStdout.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/TestResults.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/TestRunner.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/TestSuite.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/ThrowingTestReporter.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/TimeConstraint.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/TimeHelpers.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/UnitTest++.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/UnitTestPP.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/XmlTestReporter.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/Posix/SignalTranslator.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/include/UnitTest++/Posix/TimeHelpers.h
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/lib/cmake/UnitTest++/UnitTest++Config.cmake
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/lib/cmake/UnitTest++/UnitTest++Targets.cmake
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/lib/cmake/UnitTest++/UnitTest++Targets-noconfig.cmake
-- Installing: /mnt/c/Users/Mac/GitHub/GSL/build_linux/lib/pkgconfig/UnitTest++.pc
Updated 29/04/2017 20:58 2 Comments

Compile without exception support

Microsoft/GSL

I am trying to use this in an other project which compiles without exception support.
I would not mind substituting the exceptions with asserts, and let it fire only in debug.

currently I use only the <gsl/span> header, my problem lies in <gsl/gsl_util>.
Could we get something like: ```

if defined ( __cpp_exceptions) || \

    (defined (_MSC_VER) && defined (__CPPUNWIND)) || \
    (defined (__GNUC__) && defined (__EXCEPTIONS))
#define ABORT_THROW (x) throw x

else

#define ABORT_THROW (x) (x, std::abort())

endif

`` and then useABORT_THROW` when needed?

Updated 04/04/2017 21:55 12 Comments

Consider reducing Boolean to Integral

ericniebler/stl2

From LWG Kona review of #155. Boolean is an enormously complicated hunk of specification for what is essentially intended to allow people to return integers from comparison operators in addition to bool. LWG made the suggestion to simply define Boolean as: c++ template <class T> concept bool Boolean() { return Integral<T>(); } which achieves the same goal with two lines of spec. At the very least, it’s a reasonable idea to apply this simplification for the TS and see if anyone complains.

Proposed Resolution

Replace the entire contents of [concepts.lib.compare.boolean] with:

template <class T>
concept bool Boolean() {
  return Integral<T>();
}
Updated 18/06/2017 02:19 10 Comments

Be consistent about explicitly requiring typename constraints

ericniebler/stl2

…in concept definitions. This came up in LWG Kona discussion of D0541R1’s definition of Assignable: c++ template <class T, class U> concept bool Assignable() { return is_lvalue_reference<T>::value && // see below CommonReference< const remove_reference_t<T>&, const remove_reference_t<U>&>() && requires(T t, U&& u) { { t = std::forward<U>(u) } -> Same<T>&&; }; } which forms const remove_reference_t<U>& without any assurance that type is valid (U could be void, for example). In many other places, e.g. Readable, we explicitly require the validity of types despite that substitution failure will handle an ill-formed type similarly. We need to consistently choose one or the other, or at least develop a guideline for when we use which approach.

Updated 16/06/2017 16:41

Change the name of "implicit expression variants"

ericniebler/stl2

As defined in [concepts.lib.general.equality]/6:

Where a requires-expression declares an expression that is non-modifying for some constant lvalue operand, additional variants of that expression that accept a non-constant lvalue or (possibly constant) rvalue for the given operand are also required except where such an expression variant is explicitly required with differing semantics. Such implicit expression variants must meet the semantic requirements of the declared expression. The extent to which an implementation validates the syntax of these implicit expression variants is unspecified.

LWG in Kona was concerned that the term is confusing now that C++ has std::variant.

While we’re at it, define the new name as a term-of-art since it is used as such elsewhere in [concepts.lib].

Updated 16/06/2017 16:26

Don't try to forbid overloaded & in Destructible

ericniebler/stl2

Per LWG Kona consensus.

Proposed Resolution

(Relative to P0547R0) Change [concepts.lib.object.destructible] as follows:

template <class T>
concept bool Destructible() {
+ return is_nothrow_destructible<T>::value; // see below
- return is_nothrow_destructible<T>::value && // see below
-   requires(T& t, const remove_reference_t<T>& ct) {
-     { &t } -> Same<remove_reference_t<T>*>&&; // not required to be equality preserving
-     { &ct } -> Same<const remove_reference_t<T>*>&&; // not required to be equality preserving
-   };
}

Strike [concepts.lib.object.destructible]/p2 (“The expression requirement &ct …”) and [concepts.lib.object.destructible]/p3 (“n a (possibly const) lvalue t of type…”).

Updated 28/03/2017 18:25 1 Comments

Unreasonably slow

mikebmcl/P0267_RefImpl

I’m developing another graphics library: Anti-Grain Evolution (https://github.com/tyoma/agge). It is based on Maxim Shemanarev’s AGG, but is 3 times faster (and leaner in terms of functionality). So today I performed a simple test and it turns out path drawing in io2d is 6 times slower than I have in AGGE. Should I switch to antialias(io2d::fast) it becomes 3 times slower. But agge always draws in maximum possible quality (it does analytic antialiasing).

This is how I draw path (I eliminated all the obvious memory allocations I could find in the measured interval, surface and spiral are created on resize, but never in drawing cycle):

     timings.clearing += stopwatch(counter);

     _surface->line_width(3);
     _surface->line_cap(io2d::line_cap::butt);
     _surface->line_join(io2d::line_join::miter);
     _surface->stroke(io2d::rgba_color(0.0, 154.0 / 255, 1.0));
     _surface->path(*_spiral);

     timings.rasterization += stopwatch(counter);

Can you tell if I’m doing something wrong or is it just Cairo that is so slow? Being stateful (which is so not C++ish) io2d could make some use of caching, I think…

Below you can see the results are quite similar (all the pixels drawn are within the tolerance of 4 levels), but the timings are hugely different.

cairo-spiral agge-spiral

Updated 10/04/2017 04:35 5 Comments

Add comments to the css files

systers/FirstAide-web

Pick only one file at a time. This is a beginner task. You can claim one css file and start working on it Make sure the format you use while adding comments is according to this https://perishablepress.com/obsessive-css-code-formatting-organization-comments-and-signatures/

Updated 20/05/2017 13:48 16 Comments

Restrict alg.general changes from P0370 to apply only to the range-and-a-half algorithms

ericniebler/stl2

..as directed during LWG review of P0370. LWG is uncomfortable with the uncertainty this wording introduces, and would prefer to “limit the scope of the potential damage” to only the deprecated range-and-a-half algorithms.

Proposed resolution

Strike para [algorithms.general]/12 which begins, “Despite that the algorithm declarations nominally accept parameters by value […]”

This change also requires transform to constrain its function parameter with CopyConstructible as in the proposed resolution of #286.

Updated 27/06/2017 00:06 10 Comments

POST, followed by immediate GET, can cause django server to hang.

FNNDSC/ChRIS_ultron_backEnd

Description:

When POST on feed instance to create a file, then immediatly trying to get the instance the new instance to inspect the status fails.

Expected:

It should return the instance with the proper status (started, success, etc.)

Reproduce:

(instance 63 is the instance that will be create by POST) http -a chris:Chris1234 POST http://127.0.0.1:8001/api/v1/plugins/2/instances/ Content-Type:application/vnd.collection+json Accept:application/vnd.collection+json && http -a chris:Chris1234 GET http://127.0.0.1:8001/api/v1/plugins/instances/63/ Content-Type:application/vnd.collection+json Accept:application/vnd.collection+json

Result:

POST OK but then the GET hangs then timeouts.

HTTP/1.0 201 Created
Allow: GET, POST, HEAD, OPTIONS
Content-Type: application/vnd.collection+json
Date: Wed, 26 Oct 2016 10:53:19 GMT
Location: http://127.0.0.1:8001/api/v1/plugins/instances/63/
Server: WSGIServer/0.2 CPython/3.5.2
Vary: Accept
X-Frame-Options: SAMEORIGIN

{
    "collection": {
        "href": "http://127.0.0.1:8001/api/v1/plugins/2/instances/", 
        "items": [
            {
                "data": [
                    {
                        "name": "id", 
                        "value": 63
                    }, 
                    {
                        "name": "plugin_name", 
                        "value": "pacsquery"
                    }, 
                    {
                        "name": "start_date", 
                        "value": "2016-10-26T10:53:19.638699Z"
                    }, 
                    {
                        "name": "end_date", 
                        "value": "2016-10-26T10:53:19.638742Z"
                    }, 
                    {
                        "name": "status", 
                        "value": "started"
                    }, 
                    {
                        "name": "owner", 
                        "value": "chris"
                    }
                ], 
                "href": "http://127.0.0.1:8001/api/v1/plugins/instances/63/", 
                "links": [
                    {
                        "href": "http://127.0.0.1:8001/api/v1/63/", 
                        "rel": "feed"
                    }, 
                    {
                        "href": "http://127.0.0.1:8001/api/v1/plugins/2/", 
                        "rel": "plugin"
                    }
                ]
            }
        ], 
        "links": [], 
        "version": "1.0"
    }
}


http: error: Request timed out (30s).
Updated 28/03/2017 16:30 1 Comments

not_null has converting constructors?

Microsoft/GSL

It seems to me that offering a conversion from T to not_null<T> looses the potential to detect certain bugs at compile-time.

if I have a function that returns a raw (potentially null) pointer, and I carelessly pass it to a function taking not_null it will compile fine, and will try report a bug at run-time when it is likely too late.

Instead, if the constructor from T were explicit, an inadvertent assignment:

use_ptr(make_ptr());

would be impossible, and I would be forced to explicitly require a potentially unsafe conversion:

use_ptr(not_null<T>{make_ptr()});

This would be a kind of the signature: by writing this cast, I am taking the responsibility for guaranteeing that the raw pointer will not be null. If it is not the case, you will know that I did it consciously.

See also CppCoreGuidelines issue: https://github.com/isocpp/CppCoreGuidelines/issues/767

Updated 04/05/2017 10:47 17 Comments

rule suggestion T.12x Avoid recursion in variadic templates

isocpp/CppCoreGuidelines

Practical C++ Metaprogramming by @edouarda and @jfalcou highlighted a point that I think makes a good core guideline: avoid recursion in variadic templates.

This both dramatically improves compilation times and avoids recursive instantiation depth limits. The book talks about using pack expansions and std::index_sequence where possible, but perhaps there could be a note that even when those can’t be used, it is sometimes possible to rewrite recursion to log(n) depth.

Updated 21/04/2017 14:45 9 Comments

Usage documentation for GSL

Microsoft/GSL

In trying to consume the GSL library in some production code (maybe not really recommended yet?), and I’ve found that its a lot more difficult than expected due to the lack of documentation.

I think for the most part the intent of classes like span and byte and things are obvious to those of us interested in gsl. The actual implementation and usage in real code is another story though. The C++ Core Guidelines don’t provide a succinct summary of the classes that they recommend. For example span has 88 mentions, most of which are in “enforcement” blocks, prior to ever defining what a span is. Or more importantly, what it is not. byte isn’t mentioned at all, and I had to read through the issue list pretty closely here to realize there is no practical way to initialize it after beating my head on that problem for a while.

Most of the information on gsl classes can be found, but it’s spread across numerous draft papers, and revisions (or proposed or planned revisions) are not transparent to people outside of the c++ working groups.

I think either the Core Guidelines need a preface for new/proposed classes and their usage, or the gsl implementation library does. Maybe a mix of both. It would be especially helpful if it came with known issues or limitations. Even if the gsl is not supposed to be ‘production ready’ right now, it would be helpful in terms of evaluation and contribution. The answer to #348 may also bring some clarity to this.

Updated 08/05/2017 17:20 6 Comments

unique_copy and LWG2439

ericniebler/stl2

The declaration of unique_copy is underconstrained when the iterator category of the source range is not forward. It should probably be brought into line with the resolution of LWG2439.

Proposed Resolution

Update the declarations of unique_copy in the synopsis of <algorithm> in [algorithms.general] as follows: ```diff template <InputIterator I, Sentinel<I> S, WeaklyIncrementable O, class Proj = identity, IndirectRelation<projected<I, Proj>> R = equal_to<>> - requires IndirectlyCopyable<I, O>() && (ForwardIterator<I>() || - ForwardIterator<O>() || IndirectlyCopyableStorable<I, O>()) + requires IndirectlyCopyable<I, O>() && + (ForwardIterator<I>() || + (InputIterator<O>() && Same<value_type_t<I>, value_type_t<O>>()) || + IndirectlyCopyableStorable<iterator_t<Rng>, O>()) tagged_pair<tag::in(I), tag::out(O)> unique_copy(I first, S last, O result, R comp = R{}, Proj proj = Proj{});

template <InputRange Rng, WeaklyIncrementable O, class Proj = identity, IndirectRelation<projected<iterator_t<Rng>, Proj>> R = equal_to<>> requires IndirectlyCopyable<iterator_t<Rng>, O>() && - (ForwardIterator<iterator_t<Rng>>() || ForwardIterator<O>() || + (ForwardIterator<iterator_t<Rng>>() || + (InputIterator<O>() && Same<value_type_t<I>, value_type_t<O>>()) || IndirectlyCopyableStorable<iterator_t<Rng>, O>()) tagged_pair<tag::in(safe_iterator_t<Rng>), tag::out(O)> unique_copy(Rng&& rng, O result, R comp = R{}, Proj proj = Proj{}); ```

Updated 19/06/2017 17:34 6 Comments

C.120 harmful?

isocpp/CppCoreGuidelines

Because it emphases mapping of “ideas” and “concepts” onto a class hierarchy. I have never seen an example where this was beneficial; and I’ve seen many examples where it made a codebase much worse by coupling structures that represented mathematically-related things, while being unrelated (or not usefully related) within the code.

Updated 17/04/2017 19:05 3 Comments

CP guidelines for condition variables

isocpp/CppCoreGuidelines

Sorry this isn’t complete, I’ll try to flesh it out.

Condition variables are not semaphores. Notifications will be missed if they are sent when no other thread is blocked waiting on the condition variable, so you must not just rely on notify_one() or notify_all() to signal another thread, you must always have some predicate that is tested, e.g. a boolean flag (protected by the same mutex that is used when waiting on the condition variable). With a predicate that tests some condition, even if the notification is sent before the other thread waits on the condition variable then it won’t be “missed” because the predicate will be true and the wait will return immediately.

Bad:

std::condition_variable cv;
std::mutex mx;

void thread1() {
  // do some work
  // ...
  // wake other thread
  cv.notify_one();
}

void thread2() {
  std::unique_lock<std::mutex> lock(mx);
  cv.wait(lock);  // might block forever
  // do work ...
}

Good:

std::condition_variable cv;
std::mutex mx;
bool ready = false;

void thread1() {
  // do some work
  // ...
  // make condition true:
  std::lock_guard<std::mutex> lock(mx);
  ready = true;
  // wake other thread
  cv.notify_one();
}

void thread2() {
  std::unique_lock<std::mutex> lock(mx);
  cv.wait(lock, []{ return ready; });
  // do work ...
}
Updated 18/05/2017 08:34 16 Comments

NL.16: Is initialization order a noteworthy exception to the rule?

isocpp/CppCoreGuidelines

NL.16 recommends that members be laid out in public, protected, private order.

Like many people, I do this as a matter of habit without giving it any second thought. Sometimes however the initialization order of members requires me to break this rule.

Something like:

class Foo {
private:
  int handle;
public:
  Foo () :
    handle(some_func()),
    width(handle_width(handle)),
    height(handle_height(handle))
  {}
  const int width, height;
};

… where width and height’s initializers require handle to be set.

Is this worth stating as an exception?

Updated 15/05/2017 18:37 1 Comments

NL 10: suggest actual stdlib-conventions

isocpp/CppCoreGuidelines

Especially NL 10 already provides some guidance with regards to picking naming conventions if you have a choice. I would like for them to actually recommend the usage of stdlib-naming conventions in the hope of providing clear instructions for those who search for them. Picking the namingconventions of the standard-library is the only non-arbitrary choice and I believe that doing so is worth it for the following reasons: - Consistency. Almost everyone is going to use the stdlib and many are going to use boost. Therefore the only way to introduce as much consistency to the C++-world as possible, is to actually recommend those. - It will allow the distinction of functions from function-templates, once Concepts are wildly used. Without following this recommendation, it will be close to impossible to tell which it is from looking at it. This get’s even more important if we keep in mind that templates should be defined in the header - The rationale “do, whatever the standard-library of your language does” is easy to explain, intuitive and followed in almost every other language, therefor recommending it, is only consequent. - (In a language where objects can also serve as functions, the lines are somwhat blurred to begin with, making a distinction via the name somewhat less important)

As far as I see, these are all the arguments for one style that are not “but I like $STYLE better”.

I don’t think that adding that to the guidelines will make them in any way more problematic, as NL 10 is already mentions that it only applies if there is a choice. It may however help in getting to the point where one day, maybe, projects may have code with an entirely consistent style (which is the case in almost every other language, I want it in C++ too).

Updated 15/05/2017 18:44

F.16 F.18 Consume and input parameters

isocpp/CppCoreGuidelines

Rule F.18 suggests you should always pass parameters by X&& and std::move them. However, in the case where you have a function that receives multiple parameters and it keeps a copy of all of them (a constructor, maybe), I believe the better way is to pass them by value and then std::move them. This also saves you the trouble of adding a Cartesian product of overloads with const X&.

For example:

class Foo {
    Foo(std::vector<int> a, std::vector<int> b): _a(std::move(a)), _b(std::move(b)) {}
private:
    std::vector<int> _a;
    std::vector<int> _b;
};
Updated 15/05/2017 19:31 7 Comments

Liftetimes: shared_ptr questions

isocpp/CppCoreGuidelines

I’m trying to better understand how the lifetime analysis deals with shared_ptrs. I was not able to figure out why the following program would not compile with lifetime analysis turned on.

using namespace std;
struct B;

struct A
{
        shared_ptr<B> b;
        int k;
};

struct B
{
        unique_ptr<A> a;
};

void foo(A* a)
{
        a->b->a = nullptr;
        a->k = 5;
}

void d(A* a, shared_ptr<B> b)
{
        a->b = b;
}
void g(B* b, unique_ptr<A> a)
{
        b->a = move(a);
}

int main()
{
        shared_ptr<B> b = make_shared<B>();
        auto a = make_unique<A>();

        a->b = b;
        d(a.get(), b);
        g(b.get(), move(a));
        foo(b->a.get());
}
Updated 15/05/2017 18:55 8 Comments

F.40 : "Use a lambda.." : incorrect assertion that function objects don't overload?

isocpp/CppCoreGuidelines

The text says: “On the other hand, lambdas and function objects don’t overload; if you need to overload, prefer a function..”.

I was confused by this. The operator() for a “function object” is overloadable. e.g:

struct foo { void operator()(int) { std::cout << “first” << std::endl; } void operator()(double) { std::cout << “second” << std::endl; } };

Can the text be clarified?

Updated 16/06/2017 07:14 2 Comments

C.131 Avoid trivial getters and setters

isocpp/CppCoreGuidelines

One reason people write trivial getters and setters is to allow some future modifications without changing client code. The note in this item says, “A getter or a setter that converts from an internal type to an interface type is not trivial (it provides a form of information hiding).” A future change might be to change an internal type to a calculated type. Or to log every time the setter is called.

I also see this proposal http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n3888.pdf (Herb Sutter is an author), includes getters and setters such as: void set_tolerance(double tolerance); double get_tolerance(); I can’t see the implementation so I don’t know if that’s an internal type. But that’s the point - as a client of this code, I shouldn’t be exposed to whether it’s an internal type or not.

Updated 19/06/2017 18:49 19 Comments

Suggestion: ES.63: Distinguish between logical and bitwise comparison

isocpp/CppCoreGuidelines

Edit: Added text to Exceptions section, due to @BjarneStroustrup’s comment

ES.63: Distinguish between logical and bitwise comparison

Reason:

Say what you mean. If you want both the first and second term to be true, use operator&&. If you want to match bitflags, use operator&.

Matching bitflags can only be done sensibly using operator&, where doing a logical comparison can be done using one and two ampersands. In this case, use operator&&. There is no run-time speed to be gained using operator& instead.

The same story applies to operator| and operator||.

Examples:

Both terms being some value:

if (a == 42 && b == 3.14) { /* */ } //OK
if (a == 42 & b == 3.14) { /* */ } //Bad, will give compiler warning
if ((a == 42) & (b == 3.14)) { /* */ } //Bad, say what you mean

Bitflag:

if (i & flag_is_visible) { /* draw */ } //OK
if (i && flag_is_visible) { /* draw */ } //Runtime error

Alternatives: none

Exceptions:

If you don’t need the order dependence of && and you’ve measured that & is a significant performance optimization. If this is the case, it should be marked as an optimization. One way to do so is by making it an inline function, e,g, fast_and(0<=a,a<max).

Enforcement: compilers already give warnings

See alsos: none

Notes:

I saw the a == 42 & b == 3.14 in the wild, that’s why I suggested this rule, as the current guidelines do not state the preferred form explicitly yet.

Discussion:

Perhaps this item would better be renamed to a Non-Rule: NR x: operator& does not outperform operator&& in runtime performance

Updated 16/06/2017 11:18 6 Comments

SEC-3192: HSTS preload token

spring-projects/spring-security

Oleksandr Golonzovskyi (Migrated from SEC-3192) said:

HSTS header provide agent with a hint to use SSL for specific domain. However first access to a specific domain is a potential attack surface. Predefined list is maintained by Chrome and used by other browsers to avoid this vulnerability.

In order for domain to be added in preload list we need to have “preload” token in HSTS header. See: https://www.chromium.org/hsts/, https://hstspreload.appspot.com/

This token is not mentioned in original RFC, however is a recommended per https://www.owasp.org/index.php/HTTP_Strict_Transport_Security

Proposal is to add “preload” token to HSTS headers configuration, not sure if enabled by default (as it may cause long term consequences, so could require conciseness decision ). Therefore default HSTS header example after change: Strict-Transport-Security: max-age=31536000; includeSubDomains; preload

Possible additional check - allow token preload addition only if max-age >= 10886400

References: https://www.chromium.org/hsts/ https://blog.mozilla.org/security/2012/11/01/preloading-hsts/

Updated 28/04/2017 06:48 1 Comments

NL.n Use namespaces, do nest your namespaces. Do not use a suffix or prefix in all defined names to provide scope.

isocpp/CppCoreGuidelines

Reason: Provides a narrow context where defined names are short, external names are well marked and also naturally segments your header-files, this makes larger code sets more easy to browse and comprehend and aids with refactoring.

Chances are your IDE provides a folding mechanism for namespaces, which is great when browsing these larger pieces of code.

Examples:

Alternatives:

Exceptions: extern “C”

Enforcement: Lint defined names and suggest shortening of long names, find repeated prefixes.

See also:

Notes: Do not use reversed Internet domain names as namespaces, as java frequently do for package names.

Discussion: Maybe GitHub could be persuaded to provide a global top-namespace allocatioin mechanism ?

Updated 19/06/2017 18:46 19 Comments

SEC-3190: RememberMe cookies cant handle username containing colon (":")

spring-projects/spring-security

Jeremy Waters (Migrated from SEC-3190) said:

I have confirmed this issue with TokenBasedRememberMeServices. The remember me cookie is a string of the form:

username + “:” + expiryTime + “:” + Md5Hex(username + “:” + expiryTime + “:” + password + “:” + key)

This is 3 tokens seperated by colons. sample:

doejohn:1767839462751:494b6435d6d0c2146bc608782543f805

When the username contains a colon, which is the default with spring-social, cookie decoding fails as it encounters 4 tokens (splitting the username into 2 separate tokens). sample:

twitter:1705891096:1767839462751:494b6435d6d0c2146bc608782543f805

It appears there is an existing hack to deal with urls containing colons (“https://…”) in AbstractRememberMeServices.decodeCookie(). I suggest urlencoding the value before creating the cookie string; and the url decoding the token when later retrieved from the cookie.

Updated 15/06/2017 20:26 1 Comments

N1: myth: a raw array is orders of magnitudes faster than std::vector

isocpp/CppCoreGuidelines

Edit: reworded ‘slow == Good’ to ‘slow due to’ thanks to @NicolBolas Edit: explicitly mention that the example is thanks to an initialization flaw, thanks to @scraimer, @cubbimew and @galik Edit: mention the discussion of @scraimer, @cubbimew and @galik in the Discussion section

N.1: myth: a raw array is orders of magnitudes faster than std::vector

Reason:

The C++ Core Guidelines suggest to use a std::vector of a raw array (see ‘SL.10: Prefer using STL array or vector instead of a C array’). There is a myth that for run-time speed, one should use arrays. A std::vector can never be faster than an array, as it has (a pointer to the first element of) an array as one of its data members. But the difference in run-time speed is slim and absent in any non-trivial program.

One reason for this myth to persist, are examples that compare raw arrays with mis-used std::vectors. For example, when constructing a big std::vector without reserve, arrays can indeed be orders of magnitude faster, due to copying of the std::vector its elements when exceeding its capacity. Prefer to learn the correct workings of std::vector over falling back to the use of raw arrays.

A second reason for this myth to persist is when comparing arrays and std::vector in debug mode. Where a std::vector has mechanisms in place that will help you during debugging, array are indifferent between debug and release mode. That std::vector is slower in debug mode is due to range checks, which will save time debugging run-time errors. When doing a benchmark, do this in release mode.

Examples:

This example shows how incorrect initialization of a std::vector helps this myth persist.

This example is slightly modified from the first hit when Googling ‘benchmark array versus std::vector’ (the post is here).

Here is the benchmark code shown to be twice as slow as when using a raw array:

std::vector<Pixel> pixels;
pixels.resize(dimension * dimension); //Create elements that need to be modified
for(int i = 0; < dimension * dimension; ++i)
{
    // Modify each element
    pixels[i].r = 255;
    pixels[i].g = 0;
    pixels[i].b = 0;
}

This code is indeed twice as slow, as there are twice as much Pixels allocated. The correct equivalent would be:

std::vector<Pixel> pixels(dimension * dimension, Pixel(255,0,0)); //Create desired elements directly 

When running the benchmark in release mode, the run-time speeds are equivalent.

Note that still the comparison is unfair (in the favor of an array), as is compares a static array versus a dynamic array (in the std::vector).

Alternatives:

If you do not need resizing, std::array may suit your needs and give better performance at debug mode.

Exceptions:

There may be cases in which you need good performance in debug mode as well.

Enforcement:

See alsos: none

Notes:

Discussion:

It may be that optimizers once were better in optimizing calls to operator[] on a plain array, instead of a std::vector. Nowadays there is little evidence that this still is the case (see for example std::vector versus array decayed into a pointer

Updated 19/06/2017 18:59 20 Comments

Suggestion: when using a library, follow its philosophy, instead of your own

isocpp/CppCoreGuidelines

Edit: Thanks @galik for correcting me! Edit: More focus on the word ‘framework’, thanks to @galik

?.1: when working within a framework of a library, follow its philosophy, instead of your own

Reason:

The C++ Core Guidelines give a set of general rules. Existing libraries may have different philosophies (and good reasons for this). When within a framework of a library, follow its philosophy, instead of bending it to follow your own (or the C++ Core Guidlines its) guidelines. Doing so will increase your productivity.

Examples:

_memory management_

The STL supplies the std::unique_ptr and std::shared_ptr for memory management. The Qt library facilitates a parent-child architecture, in which a parent deletes it children.

When writing your own Qt class (a class that inherits from QObject), stay within that Qt framework: instead of using STL smart pointers (for example, std::shared_ptr), use the Qt mechanism instead (for example, QSharedPointer).

_signal-slot mechanism_

The Boost libraries have a library (Boost.Signals2) to implement a signal-slot mechanism. The Qt library has a custom own signal-slot mechanism.

When writing your own Qt class (a class that inherits from QObject), stay within that Qt framework: instead of using Boost.Signals2 classes, use the Qt signal-slot machanism.

Alternatives:

Exceptions:

Enforcement:

See alsos:

Notes:

The libraries given in the examples are just examples.

Discussion:

Updated 16/06/2017 11:25 2 Comments

ES.2x redundancy

isocpp/CppCoreGuidelines

ES.20 takes up about 10% of the text of the whole ES section. It hits a lot of good points, but doesn’t provide (stable) links to sub-points for citation in code review or the like. (Yes, you can cite the examples or notes, but those are numbered globally and will change over time.)

Relatedly: ES.21 and ES.22 basically just restate ES.20.

I’d suggest adding stable links into sub-sections of ES.20 and dropping 21 and 22 entirely. Alternatively, the text of 20 can be spread out between 21 and 22, but I think it’d be hard to ever be completely sure when to cite one or the other.

If there’s editorial consensus, I’m happy to take a stab at whatever direction you like.

Updated 19/06/2017 18:56

SEC-3131: JdbcMutableAclService can be very slow to update ACLs with many children, grandchildren etc

spring-projects/spring-security

Simon van der Sluis (Migrated from SEC-3131) said:

The JdbcMutableAclService method public MutableAcl updateAcl(MutableAcl acl) calls clearCacheIncludingChildren(ObjectIdentity objectIdentity), which recursively works it’s way through all of an ObjectIdentities children and their children etc.

We have a very hierarchical structure controlled by ACLS, when this structure gets large, and we update the ACLs of the root object, this recursion is a killer, as each call to clearCacheIncludingChildren(.) involves a DB query.

I have worked around this problem by implementing my own JdbcMutableAclService which instead of recursively clearing the ObjectIdentities simply calls aclCache.clearCache(). (Hooray for dependency injection.)

Performance improved from ~3 seconds to sub second on a smallish data set.

I’m not sure if the hammer approach to clearing the cache is suitable for all situations, but the current recursive approach isn’t either.

Perhaps JdbcMutableAclService should support different strategies for controlling the cache.

Updated 01/06/2017 21:14 2 Comments

SEC-3123: Encrypted property value support

spring-projects/spring-security

sreekanth (Migrated from SEC-3123) said:

Its good to have encrypted property value support for property file, now we have to use jasypt to enable this functionality unfortunately jasypt doesn’t have support for @PropertySource (java config). Though jasypt support or not its good to have it as a part of spring or spring security framework itself, since it adds value to the end user by not fiddling with third party libraries. So primary request is to do a inbuilt support for encrypted property value in spring/spring security framework.

Updated 29/03/2017 16:12 11 Comments

SEC-3072: Provide Freemarker macro library

spring-projects/spring-security

Angel D. Segarra (Migrated from SEC-3072) said:

Spring Framework Web MVC currently ships Freemarker and Velocity macro libraries along with the JSP taglib , but Spring security ships only a JSP taglib which leaves users of the other technologies without good options out of the box. To make matters worse JspTaglibs hash in Freemarker no longer works in Spring Boot. I am requesting support parity with at least Freemarker to match Spring Web MVC.

Updated 02/06/2017 13:17 3 Comments

SEC-2997: @AuthenticationPrincipal Optional<UserDetails> principal

spring-projects/spring-security

Christopher Smith (Migrated from SEC-2997) said:

Most parameter bindings now support Optional, but @AuthenticationPrincipal doesn’t. This causes some difficulties when trying to MockMvc-test controllers that use optional authentication, since my UserDetails doesn’t have a default constructor and thus spinning up the entire Spring Security context is necessary just to get null passed to the method. Supporting Optional, which would let the MVC mapper supply the “null” on its own, would eliminate the need.

Currently, trying to use Optional with @AuthenticationPrincipal results in a null Optional.

Updated 18/05/2017 21:37 1 Comments

SEC-2914: Support @AuthenticationPrincipal on @SubscribeMapping endpoints

spring-projects/spring-security

Igor Kolomiets (Migrated from SEC-2914) said:

I’m using latest websocket/messaging related features of Spring Security 4 to secure my application’s websocket messaging endpoints. We have many @SubscribeMapping annotated methods to provide request-response style of communication that need access to current authenticated user. Currently we do this to get authenticated user:

@SubscribeMapping(“/foo”) public MyResponse foo(MyAuthenticationToken authenticationToken) { MyUser user = authenticationToken.getPrincipal(); }

Naturally I’d like to use @AuthenticationPrincipal so we can do this:

@SubscribeMapping(“/foo”) public MyResponse foo(@AuthenticationPrincipal MyUser user) {

}

But it looks like @AuthenticationPrincipal is only supported by @MessageMapping annotated methods. Can we have it work for those annotated with @SubscribeMapping?

Updated 20/06/2017 10:01 1 Comments

SEC-2856: Make cookie theft detection in remember-me service configurable because it's seriously broken

spring-projects/spring-security

Jean-Pierre Bergamin (Migrated from SEC-2856) said:

After enabling remember-me authentication for our SSO portal, people were complaining about errors they got while logging in. Those errors turned out to be CookieTheftExceptions.

After investigating quite intensively how these exceptions occured, we found that there are so many regular usecases how this can happen that this feature can be considered as really broken.

h5. Usecase 1 - Open two windows in your browser and login to the remember-me enabled web app in both windows - Close the browser - Open the browser (with the setting to re-open all previous windows) - Both windows get re-opened and both send almost simultaneously a request with the same remember-me cookie to the web app - The first request succeeds, where the second one fails (because the first already consumes the cookie) and the user is logged out

h5. Usecase 2 - Log in to the remember-me enabled web-app - Close the browser - Open the browser and visit the web-app again, which triggers a remember-me authentication - The remember-me authentication takes a while (e.g. because the AD-Server responds very slowly) and the user closes the tab - The user visits the web-app again after a while and gets a CookieTheftException and is logged out

The problem here is that the browser never got the response with the updated cookie back because the user closed the tab before.

h5. Usecase 3 - Open your remember-me enabled web-app in Chrome - Close the browser - Start entering the URL of your web-app in Chrome’s address bar and hit enter - You get a CookieTheftException and are logged out

What happens here is that Chrome already sends a request in the background while entering the URL. When hitting enter before the background request returned with a new cookie in its response, a second request with the same cookie is sent again - which leads to a CookieTheftException.

h5. Usecase 4 - The remember-me enabled web-app is an SSO (single sign-on) application where people authenticate for different other web-apps - Open different web-apps which use the SSO in different tabs - Close the browser - Open the browser again (with the setting to re-load all previous tabs) - The different web-apps in the different tabs need to re-login with the SSO app and immediately redirect to it after loading - You get a CookieTheftException and are logged out

The problem here is that all webapps redirect to the SSO app and query it almost simultaneously which leads to the CookieTheftException.

As you can see, this CookieTheftException detection makes more harm than it tries to resolve. The PersistentTokenBasedRememberMeServices should have a way to disable the cookie theft detection on demand.

Currently we “disable” the cookie theft detection by always returning a constant token data like:

public class CustomPersistentTokenBasedRememberMeServices extends PersistentTokenBasedRememberMeServices {
    public CustomPersistentTokenBasedRememberMeServices(String key, UserDetailsService userDetailsService, PersistentTokenRepository tokenRepository) {
        super(key, userDetailsService, tokenRepository);
    }

    @Override
    protected String generateTokenData() {
        // Return a constant value for the token value to avoid CookieTheftExceptions.
        return "U1WUsKXNkM0Jzpozau/BeQ==";
    }
}

The PersistentTokenBasedRememberMeServices class should be configurable to have cookie theft detection turned on or off.

Updated 03/04/2017 09:38 3 Comments

SEC-2721: SaveContextOnUpdateOrErrorResponseWrapper.SaveContextServletOutputStream does not implement Servlet 3.1 abstract methods

spring-projects/spring-security

Jon Nermut (Migrated from SEC-2721) said:

If one tries to call setWriteListener on a ServletOutputStream on a stream that has been wrapped by SaveContextOnUpdateOrErrorResponseWrapper.SaveContextServletOutputStream, you get an abstract method exception, as SaveContextServletOutputStream does not implement and delegate these new methods in the Servlet 3.1 spec:

    /**
     * Checks if a non-blocking write will succeed. If this returns
     * <code>false</code>, it will cause a callback to
     * {@link WriteListener#onWritePossible()} when the buffer has emptied. If
     * this method returns <code>false</code> no further data must be written
     * until the contain calls {@link WriteListener#onWritePossible()}.
     *
     * @return <code>true</code> if data can be written, else <code>false</code>
     *
     * @since Servlet 3.1
     */
    public abstract boolean isReady();

    /**
     * Sets the {@link WriteListener} for this {@link ServletOutputStream} and
     * thereby switches to non-blocking IO. It is only valid to switch to
     * non-blocking IO within async processing or HTTP upgrade processing.
     *
     * @param listener  The non-blocking IO write listener
     *
     * @throws IllegalStateException    If this method is called if neither
     *                                  async nor HTTP upgrade is in progress or
     *                                  if the {@link WriteListener} has already
     *                                  been set
     * @throws NullPointerException     If listener is null
     *
     * @since Servlet 3.1
     */
    public abstract void setWriteListener(javax.servlet.WriteListener listener);

It just needs to delegate them through to the wrapped object.

Updated 05/06/2017 23:32 4 Comments

SEC-2712: Allow WithSecurityContextTestExecutionListener to execute after @Before

spring-projects/spring-security

Rob Winch (Migrated from SEC-2712) said:

Hi Rob, great enhancement. Would it be possible somehow to to invoke @WithUserDetails after @Before annotated method or have the execution order configurable? I think it’d be great to be able to create a new fresh user account in some sort of @Before method and then authenticate it with @WithUserDetails. I’m trying to avoid creating new user in @BeforeClass because each @test method can alter user’s information, so I configured test to rollback transaction after each @test and create a new user before, however @WithUserDetails tries to call UserDetailsService.loadUserByUsername() before actual user was created in @Before. Any ideas? Thanks a lot!

Updated 15/06/2017 20:26 5 Comments

SEC-2409: Spring Security / Spring Data Acl Integration

spring-projects/spring-security

Rob Winch (Migrated from SEC-2409) said:

Spring Security’s ACL implementation allows users to determine if a access is allowed after the results come back from the database. This works when there is a small number of results, but breaks down when paging is necessary. Now that we have Spring Data, it would be good to provide integration with Spring Data to ensure that the query’s are automatically updated based upon the security restrictions. We could provide a default strategy that aligns with Spring Security’s ACL model.

Updated 10/06/2017 17:47 26 Comments

SEC-2224: ActiveDirectoryLdapAuthenticationProvider throws BadCredentialsException if userPrincipalName not equal to sAMAccountName + @domain

spring-projects/spring-security

Michael Solano (Migrated from SEC-2224) said:

When using the sAMAccountName for authentication via ActiveDirectoryLdapAuthenticationProvider, a BadCredentialsException will be thrown if the userPrincipalName is not the sAMAccountName with @domain post-fixed.

For example, if the sAMAccountName is “bwayne” but the userPrincipalName is “bruce.wayne@batcave.net”, authentication will fail. The createBindPrincipal method assumes the userPrincipalName will be “bwayne@batcave.net” and not “bruce.wayne@batcave.net”.

The code below shows the details of that method:

    String createBindPrincipal(String username) {
        if (domain == null || username.toLowerCase().endsWith(domain)) {
            return username;
        }

        return username + "@" + domain;
    }
Updated 26/06/2017 21:04 9 Comments

SEC-2026: MethodSecurityExpressionRoot should be a public class

spring-projects/spring-security

Clark Duplichien (Migrated from SEC-2026) said:

Similar to SEC-1691, except don’t need to specify as part of config… just need it to be public. Currently, you can extend DefaultMethodSecurityExpressionHandler and provide the reference to the expression-handler element of the global-method-security config, but even if you override the createSecurityExpressionRoot method, you can’t provide an instance that extends MethodSecurityExpressionRoot, because it’s protected. This is also inconsistent with MethodSecurityExpressionRoot, which is already public.

Updated 05/04/2017 13:36 8 Comments

SEC-972: Allow use of non-numeric (i.e. string) values for ObjectIdentity.getIdentifier() with the default JDBC implementation

spring-projects/spring-security

“Zachary Roadhouse”:https://jira.spring.io/secure/ViewProfile.jspa?name=zroadhouse said:

I am integrating the Spring Security ACL module into our application which uses a mixture of UUID and numeric primary keys as our domain object identifiers. Based on reading the interface documentation fo ObjectIdentity, I had hoped that our UUID identifiers would work with the JdbcMutableAclService implementation without any customizations to the source. This is not currently true as the JdbcMutableAclService and related classes all assume that the identifier value can be converted to a Long. This assumption is limited to a small number of points in the implementation.

This is a request to change the implementation to allow the choice of Long vs. String to be made into a configuration setting (or alternate implementation).

Updated 28/03/2017 08:21 16 Comments

Fork me on GitHub