New picklists and parsers

[balthisar]

See the commit notes, but essentially this refactors the current picklists and option parsers to remove a bit of code redundancy, but more importantly, to make it vastly easier to implement new potential option values without having to write new parsers or awkwardly use existing parsers.

There are no testbase regressions on macOS; I've not built and tested on Windows as of yet.

[balthisar]

Builds and tests cleanly on Windows 10 and Ubuntu 16.04.

[geoffmcl]

@balthisar just checked out new_picklists branch to test, in Windows 10, MSVC14, but the build FAILS???

  config.c
F:\Projects\tidy-html5\src\config.c(596): error C2065: 'ParseAutoBool': undeclared identifier [F:\Projects\tidy-html5\build\temp-553\tidy-static.vcxproj]
F:\Projects\tidy-html5\src\config.c(596): warning C4047: '==': 'ParseProperty (__cdecl *const )' differs in levels of indirection from 'int' [F:\Projects\tidy-html5\build\temp-553\tidy-static.vcxproj]

Have I missed something?

Have not had a chance to look into yet, but maybe you will see the reason immediately... thanks...

[balthisar]

Yup, I see it. I'm using MSVC 17, and for some reason the _DEBUG macro isn't enabled for me, thus the function to the non-existent ParseAutoBool isn't being flagged for me.

I've just pushed a fix. I'm not sure how to enable _DEBUG when building for Windows. For that matter, I should figure out why Clang isn't defining that macro, too.

[geoffmcl]

@balthisar starting to look into it...

One thing I remember you saying is that I think you only build the Release version... In the Release version an assert compiles to nothing...

Try -

build\Win64> cmake --build . --config Debug

Only then will an assert exist... and this error is in such an assert... thanks...

PS: Looks like you found it... but for the future I always build both Debug and Release in Windows... Do you use the above cmake to do the building... I do not think _DEBUG is the same thing...

[balthisar]

I usually build on Windows using --config Release because that's in the build instructions. I'll just remember to use Debug in the future.

[geoffmcl]

Yes, it now builds, and will get on with testing...

In fact the controlling MSVC macro is /D NDEBUG... if this is defined then there is no assert!

And I think this is the same in gcc, so would assume same in Clang... it is defined that way in some standard header...

In fact, never sure why tidy uses assert at all, since only if there is no -DNDEBUG, does assert do anything, thus is really only for the Debug build...

So we are sort of asking all developers to first build and test in the debug version... But that is perhaps another topic...

[geoffmcl]

@balthisar have now compiled and tested this branch, in Windows 10, and see no immediate problem in this merge...

A small niggle!

It would be really appreciated if you would consider opening a Feature Request prior to refactoring, and presenting a fait accompli PR, saying one can read the commit notes...

This would give us a chance to present objections, ideas, feedback, ... or nothing... before all the effort is expended... not that there would be any objection to such a change and cleanup, but would also allow for suggested improvements, enhancements, etc... that could be considered...

Just a quiet suggestion... it is already noted in the CONTRIBUTING.md, which you wrote - "it is best to create an issue before putting the effort into a pull request". Why not follow this?

Anyway, for what, at this time, after such a brief, pushed, review, looks like a good move... thanks...

[balthisar]

Thanks for having a look, @geoffmcl. This was a pre-requisite for #365 and #378 -- infrastructure -- so a feature request really wasn't appropriate. A PR gives everyone a chance to say yay or nay, otherwise it would have been included in one of the bug fixes anyway.

Oops... meant to add, sorry about the commit notes. They were nicely typed up in the PR, and then an extra commit erased them.