Add templated bitwise AND block with tests #536
Conversation
There was a problem hiding this comment.
@alexhojinpark thanks for your interest in and first contribution to GR4! 👍
The code already looks good as is and seems to be well tested. There are some minor comments (see above).
If you'd be willing, would you implement the full set of 'Boolean Operators' (5 blocks) outlined in the EPIC#161?
i.e.
This would go a long way! I like the approach of templating like done, for example, in here but this is not a must.
In any case, thanks for your PR and let me know if you are OK with implementing the other similar functions as well. Much appreciated.
Otherwise, I'd merge this as is (+ the small changes outlined above).
|
|
||
| namespace gr::basic { | ||
|
|
||
| GR_REGISTER_BLOCK(gr::basic::And, [uint8_t, int16_t, int32_t]) |
There was a problem hiding this comment.
OK, maybe it might be worthwhile to expand this compared to the GR3 version to cover all signed and unsigned integral types?
@mormj what do you think? #GSoC
There was a problem hiding this comment.
Yes, I would expand this out to at least the types supported in GR3
|
@RalphSteinhagen Thank you for the feedback! Yes, I’d be happy to continue working on implementing the full set of 'Boolean Operators' for EPIC#161. Let me know if there are any specific considerations I should keep in mind. Looking forward to contributing more! |
|
@alexhojinpark just carry on. 😊 |
|
@alexhojinpark could you perhaps rebase and (force-)push (possibly with your other additions) again? I changed the restyled bot to (hopefully) accept external public PRs. The previous rule was a bit too restrictive. To become active, you'd need to rebase to the present head in 'main' though. Thanks in advance. |
|
@RalphSteinhagen Thanks for the update, I’ve rebased and force-pushed accordingly. No new additions yet, all tests pass locally! |
|
@alexhojinpark thanks for the rebase ... at least shows that the restyled CI step is now also working for remote public forks... ... it also shows that you maybe need to apply the code formatter. ;-) See here and the CI check for details:
Can be done with your next pushes though... |
|
@alexhojinpark how did u build the gnuradio4 locally.. and set it up, can u explain ? |
|
@codblack789 Hi, For setting up GNURadio4, I’d check the Building section in the README. It walks you through dependencies and CMake setup. Once you’ve got the build dir ready ( |
|
@alexhojinpark
5.cmake -DCMAKE_BUILD_TYPE=RelWithAssert -DENABLE_BLOCK_PLUGINS=ON -DENABLE_BLOCK_REGISTRY=ON .. After that when i open the same in VS code, I get Errors near header files !! No idea where am I going wrong !! |
|
|
Hey there! I noticed the CI checks are failing for formatting (Restyled) and Sonar. Since I don’t have permission to push changes to your branch(@RalphSteinhagen :( ), I wanted to offer some pointers: Restyled (clang-format): You can apply the suggested formatting locally by running something like: This should resolve the restyled check. Sonar: Sometimes forks can’t access Sonar credentials, so it may be normal for that check to fail from an external PR. You might ask the maintainers(@mormj, @wirew0rm) if it can be ignored or if they have a workaround. Let me know if you need more details—happy to help walk you through the steps! |
|
@KrxGu and @alexhojinpark alternatively you can check the GitHub restyled action output that indicates what and how to apply missing restyled diff. Small edits aside, we cannot fix remote branches (N.B. @alexhojinpark is pushing from his forked repo). That's normal ... |
|
Thank you for the suggestions. I'll make sure to follow the restyled format, and push with all other |
Signed-off-by: Krish Gupta <[email protected]>
alexhojinpark
force-pushed
the
alexhp
branch
2 times, most recently
from
e15a9a6 to
ca09fd4
Compare
|
@alexhojinpark do you plan to continue working on this PR? If yes, you may need to rebase against the latest main commit. |
Signed-off-by: alexhojinpark <[email protected]>
Signed-off-by: alexhojinpark <[email protected]>
Signed-off-by: alexhojinpark <[email protected]>
Add new logical blocks (AndConst, Not, Or, Xor) to CMakeLists. Update test files to include new blocks. Signed-off-by: alexhojinpark <[email protected]>
…d Xor with corresponding tests. Update CMakeLists to include new tests for these blocks. Signed-off-by: alexhojinpark <[email protected]>
Signed-off-by: alexhojinpark <[email protected]>
Signed-off-by: alexhojinpark <[email protected]>
|
@RalphSteinhagen Yes, I’ve rebased and force-pushed accordingly! |
Refs #161
Ported the bitwise AND functionality from GNU Radio 3.10 to GNU Radio 4 as a templated
Andblock, enabling support for multiple integer types via templates.The implementation is covered by Boost.UT unit tests, validating basic operations, edge cases, and boundary conditions.
Changes:
blocks/basic/include/gnuradio-4.0/basic/And.hpp,blocks/basic/test/qa_And.cppblocks/basic/CMakeLists.txt,blocks/basic/test/CMakeLists.txtThe current tests cover key scenarios. Should I extend them to include additional bit patterns?
Updated (2025-04-18):
AndConst.hpp,Not.hpp,Or.hpp, andXor.hppqa_AndConst.cpp,qa_Not.cpp,qa_Or.cpp, andqa_Xor.cppblocks/basic/CMakeLists.txt,blocks/basic/test/CMakeLists.txt