goatpig
More shadow warning cleanup
Before this one is accepted, please let me know so that I can check for any new warnings that have come up recently. I'm rebasing whenever new commits are made. There could be new warning/errors introduced by updated code, though. I'll need to parse all that before this is truly ready to go. Won't take long, mind you.
Will do. Tried to do it awhile back and screwed something up in Git. Will fix, retest, and let you know when it's ready.
Ready to go. Verified that the Armory code removes the warnings. LMDB and Crypto++ still have the warnings.
@goatpig - I don't think this breaks anything that wasn't already broken. After getting the tests going again (PR #173), I noticed that nothing was broken that wasn't already broken. That said, on OSX at least, the tests still freeze at BlockUtilsBare.FCGIStack. (I think we discussed this awhile back and never resolved it.) So, I can't confirm but I think this is fine.
That said, I did notice that one change or another broke this PR recently. Just fixed up this PR so that it compiles again.
Maybe the autotools migration will fix the fcgi error this time around. At any rate, I'll expect you to rebase this once OSX builds anew and you can test the code again (which reminds me I have to autotoolize the unit tests now!)
Anyways, sorry about that, this PR has to sit on the back burner a little longer.
@goatpig - Okay. I've been keeping it rebased as I go. I'll test it once OSX builds again.
@goatpig - The PR should be ready to go. I did add some code that probably should be discussed, though. I added some configure.ac changes that enable shadow warnings by default (and lay the groundwork for some other changes that may be useful or desirable down the road). The good news is that, as written, the shadow warnings aren't passed along to subdirectories (e.g., Crypto++).
The bad news is that, despite your (understandable) desire to not touch third-party code, we may want to clean up Crypto++. Its header files display loads of shadow warnings when compiling Armory-specific C++ code. It's really annoying and makes parsing for Armory warnings difficult. I tried to get this patched upstream but my initial patch was denied. I'd like to see the patch get put to good use elsewhere since I've already done the legwork. :) (Jeff's comment is reasonable, and probably best for the upstream code. For us, my initial approach will be fine, especially if the new Boost license prevents us from upgrading Crypto++.)
I'll have to take time reviewing this PR, the change set is large.
As for the license, all the stuff I push now is MIT, is that conflicting with the Cryptopp's new licensing? I was planning on merging in the upstream, is that off limit?
Crypto++ is under the Boost license now, as I recall. I seem to recall Alan saying awhile back that this prohibited us from migrating. I'd have to go look at the licenses again to double check.
FYI, there's no big gain in merging in the upstream just yet. There have been a few point upgrades but there's nothing large. 6.0 (no idea when it'll be released) would be more worthwhile. It merges in a more comprehensive version of my RFC 6979 patch (I rewrote it from scratch last year and got it upstreamed) and some other stuff that might be worthwhile. It will redo the build system, though. I'm not sure how that would be handled (i.e., if we use theirs or keep the new files) but it would probably make things more consistent. A lot of cruft is being removed, documentation improved, etc.
@goatpig - On second thought, Boost and MIT seem to be fine. I went through some old chat logs and can't find any explicit reason why the Boost license spooked Alan. I think it was just one of those things where it was a different license, and nobody wanted to take the time to explicitly confirm whether or not it'd cause problems down the road.
After spending a few minutes poking around, I think upgrading Crypto++ would be fine. Crypto++ would still be in its own little world. There don't seem to be any explicit issues that jump out. I'm sure a lawyer somewhere could nitpick but that's true for damn near every license out there. The only recurrent issue I could find is that some people seem to think the Boost license could cause issues when it's used for languages like Python and Java, where there's no explicit binary code. That wouldn't be a problem in our case.
All that said, I don't think there's a compelling reason to upgrade Crypto++ just yet. We'd have to port over our RFC 6979 patch and figure out how to play nice with their new build system. It's not difficult, just time-consuming, and I'm a bit burned out right now after wrestling with our Autotools upgrade. If 0.96 isn't in "pencils down" mode and focusing on bug fixes, it should get there really soon. 0.97 would make more sense for an upgrade.
Gaaaaaaah. Rebased and didn't realize I was rebasing on the wrong branch. Will try again in the morning.
Rebased. Would still like to see this get into Armory 0.97. The cleaner we can make the C++ code, the better off we are in the long run. :) (That said, I haven't run this patch in awhile and probably need to do a fixup before it's fully reviewed.)