[ACE6_TAO2] Fixed warnings and updated bison-generated code for ACE svc.conf #2336
Conversation
Using a modern version of GNU Bison
WalkthroughThis pull request implements a broad series of refactoring and modernization updates across both ACE and TAO code modules. A new Changes
Sequence Diagram(s):Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
TAO/tao/HTTP_Parser.cpp (1)
58-58: LGTM! The explicit cast improves type safety.The static_cast from int to u_short makes the type conversion explicit, which is good practice. However, consider adding range validation for the port number.
Add range validation to ensure the port number is valid (0-65535):
- port = static_cast<u_short> (ACE_OS::atoi (ptr + 1)); + int port_val = ACE_OS::atoi (ptr + 1); + if (port_val < 0 || port_val > 65535) { + if (this->opterr) + ACELIB_ERROR ((LM_ERROR, + ACE_TEXT ("%s: invalid port number %d\n"), + this->argv_[0], port_val)); + return 0; + } + port = static_cast<u_short> (port_val);ACE/ace/Svc_Conf.h (1)
51-51: LGTM! Consistent renaming of Bison-related identifiers.The changes improve code clarity by consistently prefixing Bison-generated identifiers with
ACE_. This helps prevent potential naming conflicts and makes the code more maintainable.The consistent prefixing of identifiers with
ACE_is a good practice for large codebases as it:
- Prevents naming collisions with other libraries
- Makes it clear which components belong to the ACE framework
- Facilitates easier maintenance and debugging
Also applies to: 59-71, 72-72, 84-84
TAO/TAO_IDL/util/utl_string.cpp (1)
88-88: LGTM! Improved readability of loop conditions.The changes replace negation operators (
!) with explicit comparisons (0 ==and0 !=). This makes the code more readable and the intent clearer.Consider extracting these string comparison operations into separate helper functions for better reusability and maintainability. For example:
bool is_chars_equal(char lhs, char rhs) { return 0 == static_cast<int>(lhs) - static_cast<int>(rhs); } bool is_chars_equal_case_insensitive(char lhs, char rhs) { return 0 == static_cast<int>(ACE_OS::ace_toupper(lhs)) - static_cast<int>(ACE_OS::ace_toupper(rhs)); }Also applies to: 108-109, 173-173
ACE/ace/Svc_Conf_Token_Table.h (1)
48-73: LGTM! Improved token definitions using enum.The replacement of macro definitions with an enumeration improves type safety and makes the code more maintainable.
Using enums instead of macros for token definitions provides several benefits:
- Type safety through strong typing
- Better debugger support
- Scope containment
- Prevention of macro expansion issues
ACE/ace/Svc_Conf_y.cpp (1)
267-303: Consider using std::size_t for memory allocation sizes.Using
ACE_YYSIZE_Twhich maps to platform-specific size types is good, but consider usingstd::size_tfor better C++ standard compliance and portability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
.gitattributes(1 hunks).github/workflows/linux.yml(2 hunks)ACE/ace/Asynch_IO_Impl.inl(1 hunks)ACE/ace/CDR_Base.cpp(11 hunks)ACE/ace/CDR_Base.h(1 hunks)ACE/ace/CDR_Base.inl(2 hunks)ACE/ace/Configuration.cpp(2 hunks)ACE/ace/Get_Opt.cpp(1 hunks)ACE/ace/Handle_Ops.cpp(2 hunks)ACE/ace/ICMP_Socket.cpp(1 hunks)ACE/ace/OS_NS_string.cpp(2 hunks)ACE/ace/OS_NS_sys_sendfile.cpp(1 hunks)ACE/ace/SPIPE_Connector.cpp(1 hunks)ACE/ace/Shared_Memory_MM.cpp(1 hunks)ACE/ace/Shared_Memory_MM.inl(1 hunks)ACE/ace/Svc_Conf.h(2 hunks)ACE/ace/Svc_Conf.y(7 hunks)ACE/ace/Svc_Conf_Lexer.cpp(3 hunks)ACE/ace/Svc_Conf_Lexer.h(1 hunks)ACE/ace/Svc_Conf_Param.h(2 hunks)ACE/ace/Svc_Conf_Token_Table.h(2 hunks)ACE/ace/Svc_Conf_y.cpp(15 hunks)ACE/ace/WIN32_Asynch_IO.cpp(1 hunks)ACE/ace/svcconfgen.mpc(1 hunks)TAO/TAO_IDL/ast/ast_decl.cpp(2 hunks)TAO/TAO_IDL/ast/ast_generator.cpp(1 hunks)TAO/TAO_IDL/ast/ast_module.cpp(1 hunks)TAO/TAO_IDL/fe/fe_declarator.cpp(1 hunks)TAO/TAO_IDL/util/utl_scope.cpp(1 hunks)TAO/TAO_IDL/util/utl_string.cpp(3 hunks)TAO/tao/AnyTypeCode/TypeCode_CDR_Extraction.cpp(4 hunks)TAO/tao/HTTP_Parser.cpp(1 hunks)TAO/tao/IIOP_Endpoint.cpp(1 hunks)TAO/tao/LF_CH_Event.cpp(2 hunks)TAO/tao/MCAST_Parser.cpp(1 hunks)TAO/tao/ORB_Core.cpp(3 hunks)TAO/tao/Object.cpp(4 hunks)TAO/tao/PortableServer/ServantRetentionStrategyRetain.cpp(6 hunks)TAO/tao/PortableServer/Servant_Base.cpp(1 hunks)TAO/tao/Profile.cpp(1 hunks)TAO/tao/Storable_FlatFileStream.cpp(2 hunks)TAO/tao/Valuetype/ValueBase.cpp(3 hunks)
✅ Files skipped from review due to trivial changes (13)
- .gitattributes
- TAO/TAO_IDL/fe/fe_declarator.cpp
- TAO/tao/Object.cpp
- TAO/tao/Profile.cpp
- ACE/ace/ICMP_Socket.cpp
- TAO/TAO_IDL/ast/ast_decl.cpp
- ACE/ace/OS_NS_string.cpp
- TAO/tao/IIOP_Endpoint.cpp
- TAO/tao/AnyTypeCode/TypeCode_CDR_Extraction.cpp
- TAO/tao/PortableServer/ServantRetentionStrategyRetain.cpp
- TAO/TAO_IDL/ast/ast_generator.cpp
- ACE/ace/Configuration.cpp
- ACE/ace/Asynch_IO_Impl.inl
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: ubuntu-24.04 g++-10 CodeQL
- GitHub Check: ubuntu-24.04 g++-10 wchar
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: ubuntu-24.04 clang++-16
- GitHub Check: VS2019Debug64Cxx20
- GitHub Check: VS2019Debug64Cxx17
- GitHub Check: ubuntu-24.04 g++-14
- GitHub Check: VS2019Release32
- GitHub Check: ubuntu-24.04 g++-13
- GitHub Check: macos-13-C++11
- GitHub Check: VS2019Release64
- GitHub Check: ubuntu-24.04 g++-11
- GitHub Check: ubuntu-22.04 g++-9
- GitHub Check: VS2019Debug64
- GitHub Check: VS2019WChar
🔇 Additional comments (44)
ACE/ace/SPIPE_Connector.cpp (1)
83-83: LGTM! The explicit cast improves type safety.The static_cast from int to mode_t makes the type conversion explicit, which is good practice for system calls like open().
ACE/ace/Handle_Ops.cpp (1)
28-28: LGTM! The explicit casts improve type safety.The static_cast from int to mode_t makes the type conversions explicit, which is good practice for system calls like open(). The changes are consistent with similar changes in SPIPE_Connector.cpp.
Also applies to: 39-39
ACE/ace/Get_Opt.cpp (1)
490-490: LGTM! The explicit cast improves type safety.The static_cast from int to ACE_TCHAR makes the type conversion explicit, which is good practice when calling character classification functions like ace_isalnum().
TAO/tao/MCAST_Parser.cpp (1)
449-450: LGTM! Improved type safety with explicit casting.The change properly ensures type safety by using
static_castto convert the port value tounsigned shortafter validating it's within the valid range.ACE/ace/OS_NS_sys_sendfile.cpp (1)
35-35: LGTM! Improved safety with proper initialization.Initializing
prev_posto -1 prevents potential undefined behavior by ensuring the variable has a known invalid value before it's potentially used.TAO/TAO_IDL/ast/ast_module.cpp (1)
253-253: LGTM! Improved readability with explicit comparison.The change from double negation to explicit comparison with zero makes the pointer assignment and validation check more readable and maintainable.
TAO/tao/LF_CH_Event.cpp (1)
42-43: LGTM! Improved consistency and safety with const pointer.The changes consistently use
constpointer and combine declaration with cast, which:
- Prevents accidental modification of the pointer
- Eliminates redundant initialization
Also applies to: 139-140
ACE/ace/Shared_Memory_MM.inl (1)
35-35: LGTM! Explicit type casting improves type safety.The explicit cast of
modetomode_tusingstatic_castis a good improvement that enhances type safety and makes the code's intent clearer.ACE/ace/Shared_Memory_MM.cpp (1)
44-44: LGTM! Consistent type casting across implementations.The explicit cast of
modetomode_tmatches the change in the .inl file, maintaining consistency in type handling across the codebase.TAO/tao/ORB_Core.cpp (3)
664-664: LGTM! Improved type safety for port number handling.The explicit casting of port values to
u_shortusingstatic_castensures proper type safety for network port numbers.Also applies to: 670-670
720-730: LGTM! Clear IPv6 link local flag handling.The IPv6 link local flag handling is implemented correctly with proper type conversion and boolean logic.
890-903:❓ Verification inconclusive
Verify logging strategy event handler registration.
The logging strategy event handler registration involves dynamic casting and reactor registration. Please ensure that:
- The dynamic cast failure case is handled gracefully
- The reactor is properly initialized before registration
🏁 Script executed:
#!/bin/bash # Check for proper error handling around dynamic_cast rg -A 5 "dynamic_cast.*ACE_Logging_Strategy"Length of output: 414
Logging Strategy Event Handler Registration Check
- The dynamic cast result is safeguarded by checking for a non-null pointer (i.e.,
if (logging_strategy != 0)), which ensures that the failure case is handled gracefully.- However, the code directly calls
logging_strategy->reactor(this->reactor())without explicitly verifying thatthis->reactor()returns a valid reactor. Please double-check that the reactor is always properly initialized at this point; if there's any possibility it might be null, consider adding an explicit null-check or logging a warning.ACE/ace/CDR_Base.h (1)
340-342: LGTM! Improved type safety for Fixed class constants.Converting
POSITIVEandNEGATIVEfrom enum values tostatic const Octetmembers improves type safety by making the constants strongly typed and explicit.ACE/ace/WIN32_Asynch_IO.cpp (1)
118-119:✅ Verification successful
Verify the impact of removing base class initialization.
The base class
ACE_Asynch_Result_Implinitialization has been removed from the initializer list. While this aligns with changes inACE/ace/Asynch_IO_Impl.inl, we should verify that:
- The base class has a default constructor
- The base class state is properly initialized
Run the following script to check the base class definition:
🏁 Script executed:
#!/bin/bash # Description: Check if ACE_Asynch_Result_Impl has a default constructor # and verify its initialization requirements # Search for the base class definition ast-grep --pattern $'class ACE_Asynch_Result_Impl { $$$ }' # Search for constructor declarations rg -A 5 "ACE_Asynch_Result_Impl::"Length of output: 839
Verified Impact of Removing Base Class Initialization
I've confirmed that the base class
ACE_Asynch_Result_Impldoes have a default constructor (as defined inACE/ace/Asynch_IO_Impl.inl) and that its state is properly initialized by default. Therefore, removing its explicit initialization from the initializer list inWIN32_Asynch_IO.cpp(lines 118–119) is safe and aligns with the intended design.
- The default constructor for
ACE_Asynch_Result_Implis defined with an empty body.- No additional state or initialization appears to be required.
ACE/ace/CDR_Base.inl (2)
284-286: LGTM! Improved type safety in digit assignment.The refactoring improves type safety and readability by:
- Separating the computation into a clear intermediate step
- Adding explicit casting to
ACE_CDR::Octet
545-545: LGTM! Added explicit type casting for sign bit manipulation.The addition of explicit casting to
ACE_CDR::Octetimproves type safety when manipulating the sign bit.TAO/TAO_IDL/util/utl_scope.cpp (1)
714-718: Great improvement in const-correctness!The changes enhance type safety by:
- Preventing modification of the pointer parameter
e- Making the
ntvariable immutable after initializationACE/ace/Svc_Conf.y (3)
42-45: Good addition of MSVC warning controls.The warning pragmas help suppress specific MSVC warnings while maintaining them for the rest of the codebase.
Also applies to: 373-375
66-67: Improved parser configuration for better compatibility.The change from
%pure_parserto%define api.pureand%paramaligns with modern Bison practices.
193-193: Standardized error handling using ACELIB_ERROR.Consistent use of ACELIB_ERROR improves error reporting and maintainability.
Also applies to: 227-230, 344-348
ACE/ace/svcconfgen.mpc (2)
4-4: Improved build configuration with custom_only flag.Setting
custom_only = 1clarifies that this is a custom build target.
6-8: Enhanced file generation process.The changes improve the build process by:
- Generating header files directly with bison
- Adding better sed transformations for consistent naming
- Cleaning up temporary files properly
Also applies to: 11-11, 18-20
ACE/ace/Svc_Conf_Lexer.h (1)
37-38: Improved type consistency with ACE-specific types.The change from
YYSTYPE*toACE_YYSTYPE*better reflects the ACE namespace and improves type clarity.Also applies to: 43-43
ACE/ace/Svc_Conf_Lexer.cpp (3)
113-114: LGTM: Function signature updated to use ACE-specific type.The change from
YYSTYPEtoACE_YYSTYPEandYYLEX_PARAMtoACE_YYLEX_PARAMimproves namespace isolation and reduces potential naming conflicts with other Yacc/Bison generated code.
148-149: LGTM: Consistent type renaming in method signature.The change from
YYSTYPEtoACE_YYSTYPEmaintains consistency with the previous change.
284-285: LGTM: Consistent type renaming in method signature.The change from
YYSTYPEtoACE_YYSTYPEmaintains consistency with the previous changes.TAO/tao/Storable_FlatFileStream.cpp (1)
332-384: LGTM: Improved error handling and code clarity.The refactoring improves the code by:
- Introducing
mtimevariable for clearer state management- Better error handling with consistent initialization
- More logical code flow with clear state transitions
TAO/tao/PortableServer/Servant_Base.cpp (2)
60-64: LGTM: Simplified constructor initialization.The removal of explicit base class initialization is appropriate as the base class likely has a suitable default constructor.
66-70: LGTM: Consistent simplification in copy constructor.The change maintains consistency with the previous constructor change.
.github/workflows/linux.yml (3)
29-41: LGTM: Updated compiler versions and added C++20 support.The changes appropriately modernize the build environment:
- Added newer GCC versions (13, 14)
- Enabled C++20 support for modern compilers
- Updated to Ubuntu 24.04
42-56: LGTM: Updated Clang compiler configurations.The changes appropriately update Clang configurations:
- Removed older versions
- Added newer versions (12, 14, 16)
- Updated to Ubuntu 24.04 where appropriate
78-94: LGTM: Consistent OS updates for feature builds.The changes consistently update feature builds to use Ubuntu 24.04, maintaining uniformity across the build matrix.
ACE/ace/Svc_Conf_Param.h (2)
89-89: LGTM! Simplified destructor declaration.The removal of the
voidparameter from the destructor declaration is a good modernization step. In C++, empty parameter lists are preferred overvoidfor better readability.
130-134: LGTM! Consistent macro naming convention.The addition of
ACE_prefixed macros and updating the parameter casting maintains consistency with the project's naming conventions.ACE/ace/Svc_Conf_Token_Table.h (3)
1-36: LGTM! Updated Bison version and licensing information.The file has been regenerated with Bison 3.8.2, updating the copyright and license information appropriately.
38-46: LGTM! Added proper header guards and debug support.The addition of proper header guards and conditional debug support improves the robustness of the code.
80-80: LGTM! Added parser function declaration.The parser function declaration with the updated parameter type is consistent with other changes.
ACE/ace/Svc_Conf_y.cpp (3)
1-19: Beneficial upgrade to Bison 3.8.2.The upgrade from GNU Bison 2.3 to 3.8.2 brings important improvements in parser generation, including better error handling and modern C++ compatibility.
37-46: Good practice: Clear documentation of private implementation details.The warning comment about not relying on undocumented features, especially those prefixed with
ACE_YY_orace_yy_, helps prevent dependency on internal implementation details.
1404-1410: Simplified copy constructor implementation.The copy constructor has been simplified by removing the base class initialization, which is correct since
ValueBasehas a default constructor.TAO/tao/Valuetype/ValueBase.cpp (2)
411-424: Improved null pointer check in unmarshal operation.The condition has been updated to properly handle null pointer cases in
create_for_unmarshal(), making the error handling more robust.
1404-1410:❓ Verification inconclusive
Verify copy constructor behavior.
The simplified copy constructor for
DefaultValueRefCountBasenow only initializesrefcount_to 1. Ensure this doesn't break any inheritance requirements.Run this script to check for potential issues:
🏁 Script executed:
#!/bin/bash # Search for derived classes that might depend on base class initialization ast-grep --pattern 'class $_ : public DefaultValueRefCountBase { $$$ }'Length of output: 246
Action Required: Confirm Inheritance Compliance for the Simplified Copy Constructor
The updated copy constructor in
TAO/tao/Valuetype/ValueBase.cpp(lines 1404–1410), which now merely setsrefcount_to 1, needs careful scrutiny to ensure none of its derived classes rely on additional base class initialization. Our initial ast-grep search (with the pattern provided) did not reveal any derived classes, though warnings from non-code (documentation) files were raised due to UTF-8 issues. This limitation means the automated search may have overlooked potential uses in source files.
- Verify manually that no derived classes require extra initialization in the base copy constructor.
- Consider re-running the search while excluding directories like
TAO/docsto eliminate encoding issues (e.g., usingrg "class .*: public DefaultValueRefCountBase" --glob "!TAO/docs/*").Please confirm that this change does not negatively impact inheritance assumptions or break any derived class behaviors.
ACE/ace/CDR_Base.cpp (2)
216-219: LGTM! Improved type safety by using ACE_UINT16.The change from ACE_UINT32 to ACE_UINT16 is more appropriate since these variables store 16-bit values. This improves type safety without any risk of data loss since the values are explicitly masked to 16 bits.
775-775: LGTM! Added explicit type casting for improved safety.The changes consistently add explicit
static_cast<Octet>for assignments todigits_andscale_members across multiple methods. This improves type safety by:
- Preventing implicit conversions
- Making type conversions visible and intentional
- Helping catch potential truncation issues at compile time
Also applies to: 802-802, 934-934, 952-954, 1201-1205, 1292-1292, 1311-1311, 1366-1366, 1401-1402, 1439-1439
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
[ACE6_TAO2] Fixed warnings and updated bison-generated code for ACE svc.conf (cherry picked from commit 9f734b4) # Conflicts: # .github/workflows/linux.yml # ACE/ace/Asynch_IO_Impl.inl # ACE/ace/Svc_Conf_Token_Table.h # ACE/ace/Svc_Conf_y.cpp # TAO/tao/IIOP_Endpoint.cpp # TAO/tao/LF_CH_Event.cpp # TAO/tao/Object.cpp # TAO/tao/Storable_FlatFileStream.cpp
No description provided.