Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce user literals parsing #50

Merged
merged 6 commits into from
Oct 24, 2024
Merged

Conversation

rykovv
Copy link
Contributor

@rykovv rykovv commented Jul 5, 2024

Closes #11

Current implementation contains a 🐛 bug. Compilation will fail for invalid values captured by user literals. Such weakness puts in danger the whole purpose of the safety library. Stronger user literals parsing is needed.

This PR adds strong support for positive decimal and hexadecimal numbers. The support is provided through creation of three concepts: hex_integer and decimal_integer. These concepts match provided values with intended types and dispatch appropriate parsing function. So, additional safety is provided.

@elbeno
Copy link
Contributor

elbeno commented Jul 6, 2024

UDLs will never receive the negative sign? Unary minus is an operator, not part of a literal. https://godbolt.org/z/M1hneGfj7

@rykovv
Copy link
Contributor Author

rykovv commented Jul 7, 2024

See the point. My understanding wasn't correct. Hence, think it can be split in following steps:

  1. Remove netagive decimal integer concept
  2. Create additional minus struct but derived from unary_op in dsl/minus.hpp
  3. Specialize for Primitive. Will need to swap min with max in ival and make both negative.
  4. Does specialization for mask make sense?
  5. Add unary minus operator overload

Am I missing something?

@lukevalenty
Copy link
Contributor

See the point. My understanding wasn't correct. Hence, think it can be split in following steps:

  1. Remove netagive decimal integer concept
  2. Create additional minus struct but derived from unary_op in dsl/minus.hpp
  3. Specialize for Primitive. Will need to swap min with max in ival and make both negative.
  4. Does specialization for mask make sense?
  5. Add unary minus operator overload

Am I missing something?

I think you can just remove the negative decimal integer concept and it's fine. The library should already handle unary minus.

include/safe/detail/concepts.hpp Show resolved Hide resolved
include/safe/detail/concepts.hpp Outdated Show resolved Hide resolved
include/safe/detail/concepts.hpp Outdated Show resolved Hide resolved
@rykovv
Copy link
Contributor Author

rykovv commented Jul 10, 2024

I think you can just remove the negative decimal integer concept and it's fine. The library should already handle unary minus.

Gotcha. Haven't seen anything in dsl/minus.hpp, but found later in var.

@rykovv
Copy link
Contributor Author

rykovv commented Jul 10, 2024

Restructuring the PR.

@rykovv rykovv force-pushed the enforce-literals-parsing branch 2 times, most recently from 50e8eab to 684a8ac Compare July 10, 2024 08:02
@rykovv rykovv closed this Jul 10, 2024
@rykovv rykovv reopened this Jul 10, 2024
@rykovv
Copy link
Contributor Author

rykovv commented Jul 10, 2024

@elbeno are there any specific requirements for clang-format? Is the style based on LLVM or Google?

@rykovv rykovv force-pushed the enforce-literals-parsing branch 2 times, most recently from 6620c04 to 6def723 Compare September 26, 2024 03:45
@rykovv
Copy link
Contributor Author

rykovv commented Sep 26, 2024

@elbeno which parameters should I use to properly apply clang-format? Couldn't find clues in git-clang-format.py

@elbeno
Copy link
Contributor

elbeno commented Sep 26, 2024

@elbeno which parameters should I use to properly apply clang-format? Couldn't find clues in git-clang-format.py

Build the target(s) fix-clang-format and check-clang-format. Note that cmake finds clang-format/clang-tidy alongside the compiler it's using. The CI on this repo currently uses clang-17 for the formatting. (Which is old; this repo needs to be updated to take advantage of clang-18. But one thing at a time.)

@rykovv
Copy link
Contributor Author

rykovv commented Oct 1, 2024

@elbeno I'm getting the code formatted with clang-format / clang-tidy, alas with wrong formatting rules (default). I don't see .clang-format file anywhere in the repo, but rather included in .gitignore. If the .clang-format file is not part of the repo, how a common style is enforced?

@rykovv
Copy link
Contributor Author

rykovv commented Oct 2, 2024

I've noticed unary negation is not supported by unsigned types. Would be a topic for another PR.

@elbeno
Copy link
Contributor

elbeno commented Oct 2, 2024

This repo pulls in various CI stuff -- including .clang-format -- from https://github.com/intel/cicd-repo-infrastructure. Running cmake should produce a .clang-format file (symlinked).

@elbeno
Copy link
Contributor

elbeno commented Oct 2, 2024

With a clean repo, what I normally do is:

$ cmake -Bbuild
$ TOOLCHAIN_ROOT=/usr/lib/llvm-17 cmake --preset clang --fresh
$ ninja -C build <target>

The first cmake run sets up the symlinks from the CICD repo, including the presets file. This is only necessary once. The second cmake run is the "real" one that uses llvm-17 (installed from the script at apt.llvm.org) and the symlnked presets file. After that, you can build any target you like.

@rykovv
Copy link
Contributor Author

rykovv commented Oct 3, 2024

This method doesn't work in Windows. Looks like presets expect clang++ binaries to be in Linux-based directories (/usr/bin). You've mentioned something about WSL (Windows Subsystem for Linux). Is there a way to get everything setup there? Otherwise, I'll need to install a Linux distro for C++ dev.

@elbeno
Copy link
Contributor

elbeno commented Oct 3, 2024

WSL will work.

@rykovv
Copy link
Contributor Author

rykovv commented Oct 3, 2024

Will try to get it working with WSL

@rykovv
Copy link
Contributor Author

rykovv commented Oct 9, 2024

So, still struggling to get it working:

Ran

$ sudo apt install libc-bin
$ sudo apt install build-essential
$ llvm.sh 17 # script from apt.llvm.org
$ sudo apt install clang-tools-17 clang-format-17 clang-tidy-17

$ cd safe-arithmetic
$ cmake -Bbuild -GNinja
$ TOOLCHAIN_ROOT=/usr/lib/llvm-17 cmake --preset clang --fresh
$ ninja -C build check-clang-format # failed

The last command faild with error: Format.cmake: cannot run because clang-format and/or python not found
So, I setup up the symbolic links

$ ln -s /usr/bin/clang-format-17 /usr/bin/clang-format
$ ln -s /usr/bin/clang-tidy-17 /usr/bin/clang-tidy
$ ln -s /usr/bin/python3 /usr/bin/python

That didn't help. So, ninja -C build check-clang-format is still not working.

On the other hand, I've tried ninja -C build all
It also failed

/safe-arithmetic/test/safe/big_integer/detail/minus.cpp
In file included from /root/safe-arithmetic/test/safe/big_integer/detail/minus.cpp:1:
In file included from /root/safe-arithmetic/test/safe/big_integer/detail/properties.hpp:3:
/root/safe-arithmetic/include/safe/big_integer/detail/storage.hpp:6:10: fatal error: 'compare' file not found
    6 | #include <compare>
      |          ^~~~~~~~~
1 error generated.

@rykovv rykovv requested a review from elbeno October 17, 2024 06:41
include/safe/int.hpp Outdated Show resolved Hide resolved
@rykovv rykovv requested a review from elbeno October 24, 2024 00:11
@elbeno elbeno merged commit 1d8e612 into intel:main Oct 24, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 User defined literals should fail compilation for invalid values
3 participants