The SvStream class exports a lot of operators for both reading and writing values to binary files. We need to clean that up and create new methods eg. 'readInt32()' that return a defined default result - ie. zero for ints, false for bools, "" for strings etc. ie. perhaps: sal_Int32 readInt32(sal_Int32 nDefault = 0); We should take care as we do the change to catch those (rather few) places where the undefined result is used as a feature: ie. "if we failed to read, the default was the old value of the result". ie. this sort of thing: SvStream& SvStream::operator>>(sal_uInt16& r) { sal_uInt16 n = 0; READNUMBER_WITHOUT_SWAP(sal_uInt16, n) if (good()) { if (bSwap) SwapUShort(n); r = n; } return *this; } is really bad news for security auditing - and it'd be lovely to kill it everywhere. Similar changes for writing would also be great - again, to avoid casual changes to types causing knock-on breakage in files. It is possible that a magic CLANG plugin could auto-generate such a patch, which may be worth investigating due to the many call-sites involved.
EasyHack with code details -> NEW
Just so I understand this change, the SvStream class should stay, it is just the stream operators that should go, correct? I can reuse the other methods in the class.
> Just so I understand this change, the SvStream class should stay Yes - that's right ! :-) Almost certainly this work is best done as a clang plugin - checkout compilerplugins/README - at least we should be able to catch the most simple cases that way: when a locally scoped variable is deliberately initialized and then there is some branch afterwards on it's result. Of course, the clang thing makes it less of an 'easy' hack - so feel free to start this by hand to get a feel for what needs doing. Thanks !
I gave this change a shot on a single method and it has been quite a major pain due to the impacting code sites (I've put 40+ hours in with no end in sight). I'm going to start over on it and I would like to know if doing it in a more incremental way would be okay for the project. In short, how about these steps: 1. Create the new read and write methods, possibly with templates and specific implementations of the template. 2. Mark the stream operators with the "__attribute__((deprecated))" flag so that the compiler screams about the usage. 3. Switch the unit tests over to the new methods. 4. Push to Gerrit for a review. (Is this appropiate at this point?) 5. Start on a series of passes to fix all the warnings. 6. Push each pass up to Gerrit for review. 7. Once the warnings are clean, deleted the now unused methods. Also other than ccache is there anything else I can do to speed up compiles, every time I touch a header it seems I have to eat a 3+ hour compile.
Created attachment 75311 [details] Step 2 of my proposed process I tested out step 2 of the proposed process just to get an idea of the size of this change. By applying the following patch to the build we get 12,203 extra warnings. My question is due to the huge of the change is submitting an incremental set of patches okay? If so, I'll get started and submit step 1.
Hi Christopher: > I gave this change a shot on a single method and it has been quite a major > pain due to the impacting code sites (I've put 40+ hours in with no end in > sight). Right ! :-) prolly worth starting with a lesser used type eg. 'double' :-) > I'm going to start over on it and I would like to know if doing it > in a more incremental way would be okay for the project. Yes - that's fine; we love incremental approaches in general. > 2. Mark the stream operators with the "__attribute__((deprecated))" > flag so that the compiler screams about the usage. I think that's something you might want locally but not in the tinderbox builds some of which tend to compile with warnings-as-errors :-) > Also other than ccache is there anything else I can do to speed up > compiles, every time I touch a header it seems I have to eat > a 3+ hour compile. ccache will not help at all if there is a header change; are you using icecream ? (if you have idling lan connected hardware that is ;-) > I tested out step 2 of the proposed process just to get an idea > of the size of this change. By applying the following patch to > the build we get 12,203 extra warnings. Right - so - this is why (in general) I think it is better to write a Clang plugin - to automate the conversion of 90+% of those call-sites where we can reasonably guess that the data was not intended to be initialized to some default state in the case of read failure; and then audit the harder ones manually. > My question is due to the huge of the change is submitting an > incremental set of patches okay? If so, I'll get started and > submit step 1. Sure - doing this incrementally is fine - but -really- it would be a better use of your brain (I think) to install clang, and have a go at the compilerplugins/ directory - at least to catch the 'easy' cases :-) How does that sound ? [ and thanks so much for looking at this ! ]
Any update here? Anyway, I put fdo#39428 in see also.
I guess poking at the compilerplugins/ and the clang approach here would be really good; in theory we can automatically re-write this :-)
adding LibreOffice developer list as CC to unresolved EasyHacks for better visibility. see e.g. http://nabble.documentfoundation.org/minutes-of-ESC-call-td4076214.html for details
Removing comma from whiteboard (please use a space to delimit values in this field) https://wiki.documentfoundation.org/QA/Bugzilla/Fields/Whiteboard#Getting_Started
ASSIGNED->NEW: Hasnt changed since 2013 apparently ...
I've already done the bulk of this. There might be some remnants left in other modules of operator<< and operator>> acting on stream-like classes, but they're definitely not EasyHack material
Migrating Whiteboard tags to Keywords: (EasyHack,DifficultyBeginner,SkillCpp,TopicCleanup) [NinjaEdit]
Remove LibreOffice Dev List from CC on EasyHacks (curtailing excessive email to list) [NinjaEdit]