option to disable the '#' flag#288
option to disable the '#' flag#288stefano-zanotti-88 wants to merge 5 commits intocharlesnicholson:mainfrom
Conversation
charlesnicholson
left a comment
charlesnicholson
left a comment
There was a problem hiding this comment.
This looks great, thanks for the PR! It looks like the tests are failing, if you can fix those up I'll happily merge it.
(Also, please add yourself to the contributors file if you like!)
nanoprintf.h
Outdated
| #endif | ||
| out_spec->case_adjust = 'a' - 'A'; // lowercase | ||
| out_spec->prepend = 0; | ||
| #if NANOPRINTF_USE_ALT_FORM_MODIFIER |
There was a problem hiding this comment.
Please change these tests to #if NANOPRINTF_USE_ALT_FORM_MODIFIER == 1 (For better or worse, the rest of the file uses the == 1 convention so I'd prefer to preserve it, and if it does change, do a sweeping change all at once).
There was a problem hiding this comment.
I wrote "Removing it --ie still parsing it, but ignoring it--", whereas the actual code I committed skips the parsing too.
I'm not sure if you prefer the "parse but ignore" or the "fail parsing" route.
As I've commented elsewhere, we could also consider a different option for "everything that is disabled will not be honored, but will still be parsed".
I've corrected the code as you asked.
Also: if you find it easier to just take the code, tweak it, and commit it in your own branch, without having to accept the PR --which might require slower back and forth discussions with me--, that's perfectly fine. No need for changes to the contributors list or merging from my fork.
Finally, I have problems with running all the tests as specified in the readme, so it would be great if you could help me out here.
Running the build.py script, I get an HTTP error. I can give more details if you can help.
I don't understand how to build the tests myself, so right now I'm just working with a reduced set of tests in a separate file of my own.
Even if I don't use the python build, what am I supposed to compile all the "unit*.cc", "conformance.cc", "doctest_main.cc", and "paland.cc" ? All of them together don't compile. I managed to compile some of them, but the tests fail because of some conformance checks that compare implementation-defined behavior against my local standard library, which is not guaranteed to behave like NPF.
There was a problem hiding this comment.
Sure, I'm happy to take this as a proof of concept and do it via a local PR at my bench if that's easier for you. I can do all the test work as well if you'd like.
Nanoprintf uses a python script to kick off CMake to generate a ninja build that compiles the 2^N flag configuration explosion. It's more than I'd do for a less-configurable project, but if nanoprintf offers a flag then I need to test that every possible build combination works with or without that flag. So, the CMakeLists.txt file has a large and very nested loop for each flag, and emits a configuration target with that particular combination of flags.
You should just be able to clone npf and run ./b --paland -v - the python script looks for cmake and ninja in your system and, if they're absent, downloads them to the local transient "build" directory in your repo directory. (It doesn't attempt to install them or anything rude). Then, having either found or downloaded them, it simply runs cmake to configure and then ninja to build.
If that's not working for you, it might be a bug or platform incompatibility in the python script- I only test that it works on linux, so if you're on a different platform then it may not even work. (Again, the philosophy of "if it's not harnessed under CI, it's probably broken" shows itself!)
If you have the time and gumption, could I ask you to open an issue with output so that I can take a look? Generally most clients simply use nanoprintf as a submodule or just grab the .h file -- that's what it's there for -- so the build and test machinery has been lower priority. I'd still like it to work, though!
There was a problem hiding this comment.
Yes, it's fine if you work on your local PR.
I'll still try to run the tests, as that can speed things up.
I'm having trouble with the non-existent:
https://cmake.org/files/v3.22/cmake-3.22.2-win64-x86_64.tar.gz
which makes the build script fail.
I'll try and download cmake manually when I have time, and go on from there.
Do you think cmake 3.31 vs 4.0 should make any difference?
I'm on Windows, by the way. So I ignore "b" and run python build.py
There was a problem hiding this comment.
I just landed a PR that tests windows downloading and fixed a few issues- cmake changed its download file naming scheme and extension for windows, which explains and fixes the error you saw.
So, python build.py --download --paland -v will force the downloading and sandboxing of the latest ninja + cmake versions, and give you as close of a local experience as possible to what the build server is doing.
If you're curious, you can go here: https://github.com/charlesnicholson/nanoprintf/actions/runs/14815979224/job/41596450464?pr=300
and expand the "Build" tab to see what it's doing. Hopefully it will be the same on your computer? Please let me know if you try it out.
I'll start the PR integration work tomorrow, by the way, thanks again for all the PRs and very thoughtful discussion!
There was a problem hiding this comment.
I'm running python build.py from a cmd prompt. I have MinGW "installed" (placed somewhere reachable from the system PATH).
I just tried building from git bash, and the error is the same -- it is still finding MinGW and compiling with that.
I don't know how to run it "inside of a visual studio developer command prompt". Do you mean running python build.py in the console of a VS code installation, or of full Visual Studio? I'm not familiar with that, but I could try. Or I could download clang and get the build script to find that instead of MinGW's GCC.
There was a problem hiding this comment.
Fascinating- sorry you're going through this; you're challenging a bunch of my assumptions about windows development :) I was in videogames + windows app development for a decade or so, and having a full Visual Studio installation was something that everybody always had for any software projects, so I'm clearly taking that for granted.
When you have Visual Studio installed for C/C++ development, it installs a shortcut that will launch powershell or the command prompt with various environment variables set up. Those update your PATH to include directories that contain programs like "cl.exe" and "link.exe" which are used for command-line compilation via MSVC. When those path entries are present, CMake will discover those tools during its host-platform-interrogation phase, and say "oh, I see, I'm doing a visual studio build." You can see CI run the script that sets the environment up for 32- or 64-bit compilation mode here: https://github.com/charlesnicholson/nanoprintf/blob/main/.github/workflows/presubmit.yml#L196
There's no particular reason that nanoprintf shouldn't work with mingw-based setups; I just haven't done the work to set it up because it's not how I develop when I'm on windows!
There was a problem hiding this comment.
I had a VS Code lying around from 2020, with the ms-vscode.cpptools extension installed.
I tried building from its terminal but it still finds MinGW. I hoped it would come with the necessary tools you described, but apparently that's only with full VS.
I'll see if I can get it, though it was already really heavyweight the last time I installed it years ago.
There was a problem hiding this comment.
I've never tried it through VS Code; I always do the (yes, really heavyweight) full VS installation.
There was a problem hiding this comment.
VS's 26GB are really too much for my home connection.
Instead, I downloaded clang from here: https://github.com/llvm/llvm-project/releases
And I got CMake to find it instead, despite weird problems with Windows cmd.
However, now I get the error
[1/811] C:\Program_Files\clang\bin\clang++.exe -DDOCTEST_CONFIG_SUPER_FAST_ASSERTS -DNANOPRINTF_USE_ALT_FORM_FLAG=0 -DNANOPRINTF_USE_BINARY_FORMAT_SPECIFIERS=0 -DNANOPRINTF_USE_FIELD_WIDTH_FORMAT_SPECIFIERS=0 -DNANOPRINTF_USE_FLOAT_FORMAT_SPECIFIERS=0 -DNANOPRINTF_USE_LARGE_FORMAT_SPECIFIERS=0 -DNANOPRINTF_USE_PRECISION_FORMAT_SPECIFIERS=0 -DNANOPRINTF_USE_WRITEBACK_FORMAT_SPECIFIERS=0 -Os -std=gnu++17 -D_DLL -D_MT -Xclang --dependent-lib=msvcrt -pedantic -Wall -Wextra -Wundef -Werror -Weverything -MD -MT CMakeFiles/npf_paland.dir/tests/mpaland-conformance/paland.cc.obj -MF CMakeFiles\npf_paland.dir\tests\mpaland-conformance\paland.cc.obj.d -o CMakeFiles/npf_paland.dir/tests/mpaland-conformance/paland.cc.obj -c C:/npf/tests/mpaland-conformance/paland.cc
FAILED: CMakeFiles/npf_paland.dir/tests/mpaland-conformance/paland.cc.obj
C:\Program_Files\clang\bin\clang++.exe -DDOCTEST_CONFIG_SUPER_FAST_ASSERTS -DNANOPRINTF_USE_ALT_FORM_FLAG=0 -DNANOPRINTF_USE_BINARY_FORMAT_SPECIFIERS=0 -DNANOPRINTF_USE_FIELD_WIDTH_FORMAT_SPECIFIERS=0 -DNANOPRINTF_USE_FLOAT_FORMAT_SPECIFIERS=0 -DNANOPRINTF_USE_LARGE_FORMAT_SPECIFIERS=0 -DNANOPRINTF_USE_PRECISION_FORMAT_SPECIFIERS=0 -DNANOPRINTF_USE_WRITEBACK_FORMAT_SPECIFIERS=0 -Os -std=gnu++17 -D_DLL -D_MT -Xclang --dependent-lib=msvcrt -pedantic -Wall -Wextra -Wundef -Werror -Weverything -MD -MT CMakeFiles/npf_paland.dir/tests/mpaland-conformance/paland.cc.obj -MF CMakeFiles\npf_paland.dir\tests\mpaland-conformance\paland.cc.obj.d -o CMakeFiles/npf_paland.dir/tests/mpaland-conformance/paland.cc.obj -c C:/npf/tests/mpaland-conformance/paland.cc
In file included from C:/npf/tests/mpaland-conformance/paland.cc:62:
C:/npf/tests/mpaland-conformance\../doctest.h:978:56: error: no member named 'operator<<' in the global namespace
978 | struct has_global_insertion_operator<T, decltype(::operator<<(declval<std::ostream&>(), declval<const T&>()), void())> : types::true_type { };
| ~~^
I find it strange that this clang invocation gets the "-std=gnu++17" option rather than "-std=c++17". I can't find such an option in CMakeLists.txt
I tried adding add_compile_options("-std=c++17") somewhere in CMakeLists.txt, and it is indeed used now, but the error remains the same.
Also strange: doctest.h:978 is a line of code guarded by #if defined(_MSC_VER) && _MSC_VER <= 1900, so why is it being compiled by clang?
Building from cmd and from git bash is identical: it uses clang, and produces this error.
There is also a --dependent-lib=msvcrt in the compilation command. Could it be a cause/symptom of the problem?
* windows + mac download * fix download links, add type annotations, format * unzip * typo (cherry picked from commit 9980d63)
…_ALT_FORM_FLAG, and canonicalized related #if checks
|
Implemented in #304, mostly just copy-paste. thanks for the PR! |
3 participants
'#' has limited use ("0x/0b" prefix, single "0" for octal, "." always present for floats).
Removing it --ie still parsing it, but ignoring it-- will save some code for a feature that might not be needed at all in some code bases.
If '#' is disabled, the 'alt_form' field in npf_format_spec_t is conditionally-removed.
Because of this possibility, it is be beneficial to reorder the fields of npf_format_spec_t
For instance, on systems with 2-byte integers, this reordering can reduce the struct size from 13+1 bytes to 12 bytes, if the library is configured as:
NANOPRINTF_USE_FIELD_WIDTH_FORMAT_SPECIFIERS 1
NANOPRINTF_USE_PRECISION_FORMAT_SPECIFIERS 1
NANOPRINTF_USE_ALT_FORM_MODIFIER 0