On Thu, 2016-02-18 at 08:36 +0000, zyx wrote: > On Wed, 2016-02-17 at 23:32 +0000, Matthew Brincke wrote:
>> the attached is for removing some duplications in PdfVariant >> with the help of const_cast. > > Hi, Hi, sorry for the long (almost a month of 30 days) delay. I had some problems with backups following the discovery of the glibc vulnerability of 15 February. Here comes my reply to your (zyx') objections to my patch: > the "cast" from non-const to const does compiler for free. Using 1. In this case, it doesn't. Without explicitly giving it, the compiler would insert a call to the same method it's in, leading to endless recursion. So the cast on "this" is needed to call the correct method. > const_cast() only points to a wrong API design (if you work with a > const object, but want to modify its content (state), then you > shouldn't work with the const object at the first place). > The 'const' is also a hint for the API user, that the > function/pointer/whatever is meant for read-only purposes. The const object in question here isn't API-user-provided and also not coming from a method in another class, so this doesn't apply, IMO, because the object is only (cast to, reason see above) const to call the the same method, but with "const" qualification (to avoid duplication). Any assumptions the called code makes could be remedied in the caller (it's in the same class), such as setting a "dirty" flag, but I don't see any. > > That's at least my personal opinion on the const/non-const in general. > That's a committer's opinion, so carries special significance IMHO. > Even you are right with the code duplication, it's nothing significant. It affects *three* methods, therefore IMO is significant. > And both are expanded inline, thus no big deal either. From my point of > view. They are currently, but won't be for long if my next patch about dynamic_cast is acceptable. (I'm planning getting rid of most old-style casts in PoDoFo). > >> Sorry that only build-tested it, I couldn't do tests >> running code for security-policy reasons (my laptop is employer- >> sponsored). > > I do not know details of your deal with your employer, but if you > cannot run the code, you probably should also *not* build it. Build scripts > can run other scripts which can do "magics" with your machine. Security > speaking. > I have inline-corrected your omission (as I understand it). I had actually already audited the build "script" (CMakeLists.txt) and what gets called by it (including in-project cmake modules). The security policy I'm under allows running what I've audited (and found harmless). I just didn't have enough PoDoFo time for auditing all of it, which would be needed for run-tests with the whole library linked/loaded. > >> So please run-test it if at all possible> > > Ehm... > Sorry for not finding that (really non-) answer "Ehm" convincing for denying my request. > Bye, > zyx All in all, please reconsider your rejection and accept my apologies for going over 30 days of not posting with the formulation of this reply (in one piece). Best regards, mabri ------------------------------------------------------------------------------ Transform Data into Opportunity. Accelerate data analysis in your applications with Intel Data Analytics Acceleration Library. Click to learn more. http://pubads.g.doubleclick.net/gampad/clk?id=278785231&iu=/4140 _______________________________________________ Podofo-users mailing list Podofo-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/podofo-users
