Re: [squid-dev] [CODE] iterating over enums
On Wed, Aug 12, 2015 at 9:11 PM, Alex Rousskov <
rouss...@measurement-factory.com> wrote:
> On 08/12/2015 09:36 AM, Kinkie wrote:
>
> > So I went ahead and just implemented it, starting from your observations.
> > It's a bit more complex that your proposal, as I took the chance to
> > implement bidirectional forward and reverse iterators.
> > The attached patch is in changeset 14235 of
> > lp:~kinkie/squid/coverity-fixes, but since the code is quite well
> > self-contained I'd like to submit it for audit as a standalone unit; If
>
> Yes, submitting Enum iterators for review as a stand-alone patch is the
> right thing to do. Hiding this and other significant changes under
> "coverity fixes" cover is wrong IMO, even if those changes were prompted
> by something Coverity has detected.
>
> I would encourage you to show actual Squid usage examples (besides
> artificial test cases), either in the patch itself or in the patch cover
> email.
>
./src/HttpHdrCc.cc:HttpHdrCcType &operator++ (HttpHdrCcType &aHeader)
./src/HttpHdrSc.cc:http_hdr_sc_type &operator++ (http_hdr_sc_type &aHeader)
./src/HttpHdrScTarget.cc:http_hdr_sc_type &operator++ (http_hdr_sc_type
&aHeader);
./src/HttpMsg.cc:HttpMsgParseState &operator++ (HttpMsgParseState &aState)
./src/errorpage.cc:err_type &operator++ (err_type &anErr)
./src/http/RequestMethod.cc:operator++ (Http::MethodType &aMethod)
./src/mem/old_api.cc:mem_type &operator++ (mem_type &aMem)
These are all used to iterate over enums from what I can tell. There are
for sure more cases using different techniques, that are less trivial to
detect.
> Also, please adjust the WholeEnum documentation. It is currently too
> similar to EnumRange, including the identical first/summary line. It is
> often best to document children classes in terms of parent classes
> rather then repeating everything the parent documentation [currently]
> says IMO. For example:
>
> /// EnumRange for iterating all enum values, from enumBegin_ up to, but
> // not including, enumEnd_. The two markers must be present in the enum.
> template
> class WholeEnum ...
>
>
> If you have not considered implementing WholeEnum without inheritance,
> please do so. I speculate that lack of any data members may help inline
> WholeEnum. Please note that I am just asking you to _consider_ this. I
> do not have enough information to insist on it.
>
As all classes are nonvirtual and everything is in the same translation
unit, for now I would skip that. and avoid the code duplication.
> > +typedef typename std::underlying_type::type value_t;
> > +value_t current_value;
>
> This type and data member is more about iteration position than "value"
> (in STL sense). AFAICT, STL calls this type "iterator_type" and the data
> member "current". I think that naming is better than yours.
>
Ok
>
>
> > +reverse_iterator rbegin() const { auto i = reverse_iterator(end_);
> ++i; return i; }
> > +reverse_iterator rend() const { auto i = reverse_iterator(begin_);
> ++i; return i; }
>
> If these methods can be implemented simply as
>
> reverse_iterator rbegin() const { return ++reverse_iterator(end_); }
> reverse_iterator rend() const { return ++reverse_iterator(begin_); }
>
> then please simplify.
>
Ok.
> I do not understand how rend() would actually work when iterating whole
> enums! For a typical enum that starts with a zero-valued item, will the
> rend() position (called current_value in your code) go negative?? Is
> that actually guaranteed to work by the standard? If not, we may need to
> require a begin-1 marker (which may be negative if needed)!
>
It'll go negative, or in case of unsigned underlyng_type, wrap. It'll still
work (added a test for that), to be sure.
> Are you sure you need reverse iterators at all?
>
No, not sure; I added them mostly for completeness sake.
> [ EnumRange is not suitable for iterating up to the end of an enum that
> does not have an end+1 marker. This limitation is not a bug, but may be
> another reason to require all enums to have markers. ]
>
I've added documentation to highlight that corner case so it doesn't come
unexpected.
I've considered changing it so that it includes the last value in the
range, but in the end decided against it as it's inconsistent with common
behavior.
In some cases it may not be feasible to have markers. Maybe the enum comes
from a library, or maybe markers may hurt some code path for all we know.
As long as the behavior is well documented and consistent, I don't think
it'll be a problem.
> If possible, please add and use a global function to guess EnumRange
> template parameter value from the actual function parameter types and
> return the right EnumRange object, to avoid writing Enum type three
> times when using EnumRangeT<>.
>
> template
> EnumRangeT EnumRange(Enum begin, Enum end) {
> return EnumRangeT(begin, end);
> }
>
>
Done.
I'm attaching the resulting files. If that's OK by you, I'll start using
them and merge together wi
Re: [squid-dev] squid-3.5.7 -- xstrerror missing
On 13/08/2015 9:04 a.m., Linda W wrote: > Amos Jeffries wrote: >> On 12/08/2015 10:27 p.m., Amos Jeffries wrote: >>> I think I've cracked this. >>> >>> Both the helpers mentioned so far are missing libmiscutil.la. >>> >> >> Or not. Sorry, got a bit too excited looking at an older version. >> >> That code did move to libcompat-squid.la by 3.5.7. And your traces show >> that is being linked to the helpers that are failing. >> >> So what does this produce please ? >> nm compat/.libs/libcompat-squid.a > > Ishtar:tools/squid/squid-3.5.7> nm compat/.libs/libcompat-squid.a > > assert.o: > 0001 C __gnu_lto_slim > 0001 C __gnu_lto_v1 > ... > > mswindows.o: > 0001 C __gnu_lto_slim > 0001 C __gnu_lto_v1 > - So there is absolutely *no* portability symbols defined. Not one. > > I pulled out about every optional thing I could get away with and made > it to > what I think was a final link but LOTS of undef syms Wait. You pulled what out? how? Things which are optional to build either have ./configure --disable or --without options available. Or have advanced (-D flag) options that can be set in CXXFLAGS to disable/replace. The rest of the code is not optional. Amos ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
[squid-dev] More woes with ubuntu-precise
Hi, EnumIterator has brought out one more issue with ubuntu-precise: since it carries gcc-4.6, it doesn't support std::underlying_type. The options are: - drop support for precise in trunk. We claim to need gcc 4.8 anyway - not to use std::underlying_type and blindly use int. - do template metaprogramming to detect std::underlying_type, fall back to int if not possible - use autoconf 3. and 4. are quite crazy IMO but I mentioned them for completeness' sake. Any opinion on 1. and 2.? -- Francesco ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [CODE] iterating over enums
On 08/13/2015 03:20 AM, Kinkie wrote: > On Wed, Aug 12, 2015 at 9:11 PM, Alex Rousskov wrote: > I would encourage you to show actual Squid usage examples (besides > artificial test cases), either in the patch itself or in the patch cover > email. > ./src/HttpHdrCc.cc:HttpHdrCcType &operator++ (HttpHdrCcType &aHeader) > ./src/HttpHdrSc.cc:http_hdr_sc_type &operator++ (http_hdr_sc_type &aHeader) > ./src/HttpHdrScTarget.cc:http_hdr_sc_type &operator++ (http_hdr_sc_type > &aHeader); > ./src/HttpMsg.cc:HttpMsgParseState &operator++ (HttpMsgParseState &aState) > ./src/errorpage.cc:err_type &operator++ (err_type &anErr) > ./src/http/RequestMethod.cc:operator++ (Http::MethodType &aMethod) > ./src/mem/old_api.cc:mem_type &operator++ (mem_type &aMem) > > These are all used to iterate over enums from what I can tell. By "actual Squid usage examples", I meant examples of your new code used in Squid code (besides test cases) and not references to places where the new code might be used in the future. Seeing real usage examples, especially for unusual cases like iterating a sub-range is often very helpful when reviewing a new API. Similarly, proposing a replacement API without showing what changes it would bring to Squid sources is setting a bad example/precedent, even if these specific changes themselves are going to be welcomed by everybody. > If you have not considered implementing WholeEnum without inheritance, > please do so. I speculate that lack of any data members may help inline > WholeEnum. Please note that I am just asking you to _consider_ this. I > do not have enough information to insist on it. > > > As all classes are nonvirtual and everything is in the same translation > unit, for now I would skip that. and avoid the code duplication. I do not understand how "nonvirtual and everything is in the same translation unit" is related to eliminating data members, but I do appreciate you considering my request! > I'm attaching the resulting files. If that's OK by you, I'll start using > them and merge together with the next round of coverity-fixes. As I said, I think these changes should be posted as a regular [PATCH] and go through the normal review and commit process. "Coverity fixes" commits should be limited to trivial, small changes. However, I _personally_ may not need to review your [PATCH] if you post it, so you may ask Amos whether he needs to review it before spending time on that repost. On an unrelated note, it just occurred to me that you can save a little ink if you s/EnumIterator/Enumerator/g :-). Totally optional, of course. Thank you, Alex. ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [CODE] iterating over enums
On 08/13/2015 03:20 AM, Kinkie wrote:
> On Wed, Aug 12, 2015 at 9:11 PM, Alex Rousskov wrote:
> template
> EnumRangeT EnumRange(Enum begin, Enum end) {
> return EnumRangeT(begin, end);
> }
> Done.
> * Typical use:
> * for ( auto enumvalue : EnumRangeT(EnumType::somevalue,
> *EnumType::someOtherValue) )
> * { do_stuff(); }
Sorry, forgot to mention: This "typical usage" example should probably
be adjusted and moved to EnumRange() because using EnumRangeT directly
ought to be _atypical_.
Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] More woes with ubuntu-precise
On 08/13/2015 09:29 AM, Kinkie wrote: >EnumIterator has brought out one more issue with ubuntu-precise: > since it carries gcc-4.6, it doesn't support std::underlying_type. > The options are: > - drop support for precise in trunk. We claim to need gcc 4.8 anyway I am confused: If GCC v4.8 is required, then why are you asking about a Ubuntu release that comes with GCC v4.6? Does it imply that we were only pretending to require GCC v4.8 and now may actually have to start requiring it? > - not to use std::underlying_type and blindly use int. > - do template metaprogramming to detect std::underlying_type, fall back > to int if not possible > - use autoconf > > 3. and 4. are quite crazy IMO but I mentioned them for completeness' sake. > Any opinion on 1. and 2.? If we do not truly require GCC v4.8 yet, then I like #2 because it is simple and sufficient. [ If you have to replace std::underlying_type::type with "int" more than once, ] please add a named type so that it is easy to find such replacements. Do not just use "int" directly. typedef underlying_enum_type int; HTH, Alex. ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] More woes with ubuntu-precise
On 14/08/2015 10:18 a.m., Alex Rousskov wrote: > On 08/13/2015 09:29 AM, Kinkie wrote: > >>EnumIterator has brought out one more issue with ubuntu-precise: >> since it carries gcc-4.6, it doesn't support std::underlying_type. > > >> The options are: >> - drop support for precise in trunk. We claim to need gcc 4.8 anyway > > I am confused: If GCC v4.8 is required, then why are you asking about a > Ubuntu release that comes with GCC v4.6? Does it imply that we were only > pretending to require GCC v4.8 and now may actually have to start > requiring it? My thoughts almost the same. The published "requirement" I put up was: " This series of Squid requires a C++11 capable compiler. The currently known compilers which meet this criteria and build Squid reliably are GCC 4.9+, Clang 3.5+, and Intel CC 12.0+ " So its not even 4.8, more of a soft-ish 4.9+. If we have an easy way/wrapper/hack to allow compile on older versions do it still. If Squid happens to build (and work) on 4.8 or 4.6 great. But they are acceptible casualties if there is no choice. Note that Ubuntu will not ever support these newer Squid versions on their LTS anyway. It is entirely our choice and burden whether to support people who want the latest and greatest Squid software to work seamlessly when mixed into an ancient environment. IMO its on their shoulders to see that the dependencies are at least usable. > >> - not to use std::underlying_type and blindly use int. >> - do template metaprogramming to detect std::underlying_type, fall back >> to int if not possible >> - use autoconf >> >> 3. and 4. are quite crazy IMO but I mentioned them for completeness' sake. >> Any opinion on 1. and 2.? > > > If we do not truly require GCC v4.8 yet, then I like #2 because it is > simple and sufficient. > > [ If you have to replace std::underlying_type::type with "int" > more than once, ] please add a named type so that it is easy to find > such replacements. Do not just use "int" directly. > > typedef underlying_enum_type int; > Agreed. Amos ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
