[Misc] Add compiler warning option -Wshadow and -Wshadow-field

[ibmibmibm]

• Prevent Fd in INode leaking to Poller interface
• Remove unused Conf
  • Add warning for declaration shadowing another declaration.

[juntao]

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.

---

This compilation of summaries gives a comprehensive view of a broad set of changes in the codebase, mainly focusing on introducing new compiler warning options -Wshadow and -Wshadow-field, refactoring compiler warning flags, changing variable naming, and alterations in class inheritance.

Potential issues include the introduction of -Wshadow and -Wshadow-field flags could potentially cause a flood of warnings if the codebase has a lot of instances of shadowing, that could overwhelm developers especially if these warnings were previously suppressed. Also, the removed local Conf variable in APIAOTCoreTest.cpp could trigger unexpected behavior if its implementation is crucial inside the concerning lambda function.

Furthermore, altering the inheritance from public to private of 'FdHolder' in 'Poller' class could affect code sections depending on Poller's public interface. Another significant issue flagged is the abrupt appearance of WASMEDGE_ENABLE_UB_SANITIZER, which could cause unexpected behavior if not initialized correctly.

The summaries also highlight several variable renaming and refactoring in various classes to address shadowing warnings, possibly affecting backward compatibility and requiring adjustments in external code and documentation.

Lastly, concerns are expressed about the rationale behind removing and reordering certain warning flags and their potential consequences. For instance, removing -Wshadow could obscure shadowing issues leading to hard-to-find bugs. Similarly, the addition of -Wno-c++20-designator, -Wno-error=switch-enum, and -Wno-error=covered-switch-default could suppress important warnings.

In conclusion, while these changes appear intended to increase code robustness and improve housekeeping, their impacts could be far-reaching and potentially problematic. Detailed justifications for these changes, backed with rigorous testing and appropriate continuous integration checks, are recommended to ensure a smooth and error-free transition.

Details

Commit 6bd29c7ada492f30b101d4e05f39bfd63fc6b3f9

Summary of Key Changes:

1. Added the -Wshadow-field compiler flag in cmake/Helper.cmake. This enables warnings when a field in a class/struct declaration shadows another name, potentially preventing subtle bugs.
2. Removed the -Wno-error=shadow-field flag. This indicates that the team is no longer suppressing shadow field warnings-as-errors and is now treating them as regular warnings when encountered.
3. Modified the inheritance specification of class Poller in include/host/wasi/inode.h, changing the inheritance of FdHolder from public to private. This prevents Fd in INode from leaking to the Poller interface.

Potential Problems:

1. The change in inheritance from public to private may have downstream effects on other parts of the code that relies on Poller's public interface. If the FdHolder methods were in use directly through a Poller object, these uses are no longer valid.
2. The addition of -Wshadow-field flags can potentially introduce numerous warnings if there are any shadow fields in the existing code.
3. The removal of -Wno-error=shadow-field could allow the build to proceed where it previously failed due to shadowing issues. It might hide potential problems that would have previously prevented a build.

Commit 574562d1586902ba6ef0ea7c148eb92d9d2c6556

This pull request introduces two main changes in the code:

1. In cmake/Helper.cmake, the compiler warning option -Wshadow has been introduced along with other existing options like -Wall, -Wextra and -Wshadow-field. This option will warn about shadow declarations that occur within a function.

2. In test/api/APIAOTCoreTest.cpp, a local variable Conf is removed from a lambda function.

Now, when analyzing the potential problems or downsides:

1. The introduction of -Wshadow could lead to a lot of warnings if the existing codebase extensively contains shadowing (variables declared with the same name in different scopes). Hence, if not handled differently, this could overwhelm the developers with many warnings during the building process.

2. The local Conf variable has been completely removed in the lambda of Compile function. If this Conf variable is not used elsewhere, then it's all right. However, If it's actually needed inside the lambda function, then this can lead to unexpected behavior or even program crash depending on the usage.

In conclusion, this pull request helps add more robustness to the code by introducing the shadowing warning but it also removes an existing Conf variable, whose effect is uncertain without the context of full codebase. More tests or thorough code review is necessary to make sure these changes are safe and suitable.

Commit c9177aead1086b5c81a75c18eee9009161ebd9a6

Summary of Key Changes:

• Refactored the compiler warning options in the cmake file to make it cleaner.

Key changes:

1. -Wshadow and -Wshadow-field flags have been moved to specific compiler conditions in the CMake file.
2. Use of STREQUAL instead of MATCHES when checking the compiler type.
3. Reordered the conditions to enable the undefined behavior sanitizer (WASMEDGE_ENABLE_UB_SANITIZER) and moved the condition after the compiler checks.
4. Added an additional -Wno-error=psabi warning flag for specific compiler conditions.
5. Added a -Wno-error=return-std-move-in-c++11 warning flag for a Clang compiler version less than 13.0.0.

Potential Problems:

1. If, for some reason, the CMAKE_CXX_COMPILER_ID does not exactly match the strings "MSVC", "GNU", or "Clang", this could introduce unexpected behavior due to the use of STREQUAL instead of MATCHES. This could potentially exclude different versions or variants of these compilers.
2. The -Wshadow and -Wshadow-field flags might not be supported by all versions of all compilers, potentially leading to a warning or error during compilation.
3. The added -Wno-error=psabi and -Wno-error=return-std-move-in-c++11 warning flags might suppress potential important issues in the code.
4. WASMEDGE_ENABLE_UB_SANITIZER appears out of nowhere. If a variable is not initialized properly, that might lead to unanticipated consequences.
5. The removal of -Wno-psabi for non-AppleClang compilers might enable potentially undesired warnings.

Commit 27ff4897fe64014ae3f23a07e16572ad9d4ca7fa

The patch consists of a single change within the file lib/host/wasi/inode-macos.cpp. The author has renamed a variable from Res to Res2 within an if statement.

This modification has been made to prevent a shadowing error. In the original code, a variable with the name Res was declared twice in two different if statements. The second Res was shadowing the first one.

However, before approving this change, one should consider if renaming the variable to Res2 is really the most meaningful and clear way to handle this. Avoiding numbered variable names could improve readability and maintainability. A more descriptive name indicating the difference between the two results may be a better solution, depending on the exact context and functions of the variables involved.

Also, as this patch addresses a specific issue related to the warning option, a evaluation should be run to check whether there are other instances of variable shadowing throughout the code that would trigger warnings once the -Wshadow and -Wshadow-field options are added.

Commit 2ec110a25978f86b52936aefb4ed998faa5a3573

This pull request contains code changes related to adding a compiler warning option -Wshadow and -Wshadow-field. In addition, it fixes the compiler warning shadow-field-in-constructor. The changes have been made across 18 different files.

The primary contributor is Shen-Ta Hsieh. The change consists of 77 insertions and 75 deletions in the code. The change can be mainly categorized into two broad areas:

1. Variable renaming: Variables that were shadowing other variables in an outer scope have been renamed to prevent shadowing.

2. Refactor of constructor parameters: The variable names of constructor parameters in various classes have been modified, likely to prevent shadow warnings.

Some potential problems and things to look out for include:

1. Backward Compatibility: The names of many class member variables have been changed. This may pose problems if existing code accesses these variables directly. In such cases, refactoring will be necessary.

2. Testing: Given the somewhat extensive nature of the changes, it is important to ensure existing unit tests adequately cover the modified codeblocks. New tests may be required to confirm that the changes have not introduced new bugs.

3. Documentation: If the renamed variables are documented elsewhere (e.g., in API documentation), the documentation will need to be updated to reflect these changes.

Commit 6666b60f1b8ea0fc564085d20f3b912ae04ccf7b

Summary:
The pull request alters the compiler warning options by refactoring the WASMEDGE_CFLAGS list found within the Helper.cmake file. In the existing file, the warning flags '-Wshadow' and '-Wno-c++20-designator' are removed, and multiple warning flags including '-Wc++20-designator', '-Wno-error=switch-enum', '-Wno-error=covered-switch-default', etc. are added. Additionally, if the compiler is identified as 'Clang', a new flag '-Wno-error=reserved-identifier' is added. For the 'WASMEDGE_ENABLE_UB_SANITIZER' option, the 'fsanitize=undefined' compiler flag remains unchanged.

Potential Problems:

1. The author removes and adds certain flags without any explanation regarding the justification behind these changes. This makes it difficult to understand why the modifications were made, potentially leading to maintenance and debugging issues in the future.

2. The commit message comes across as ambiguous and might not reflect the real modifications made. For instance, it was stated that '-Wshadow' and '-Wshadow-field' are added, but '-Wshadow' is actually removed from the list.

3. The presence of '-Wno-c++20-designator', '-Wno-error=switch-enum', '-Wno-error=covered-switch-default', etc. is turning off warnings for these specific issues. In case these warnings were relevant and were turned off inadvertently, it could potentially lead to undetected problems in the code.

4. Removing the '-Wshadow' flag which provides a warning when a local variable shadows another variable could potentially lead to confusion and produce bugs that are hard to find.

5. Lastly, the movement of '-Wno-exit-time-destructors' and '-Wno-global-constructors' from the 'if(WIN32)' condition could potentially cause warnings or errors depending on different compilers or platform-specific configurations.

To conclude, it would be beneficial if the reasons behind these changes could be provided. Furthermore, appropriate code testing and continuous integration checks should take place to ensure there are no adverse effects due to these adjustments.