-
Notifications
You must be signed in to change notification settings - Fork 0
DAG: exp opcodes cannotBeOrderedNegativeFP #1
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @pranavk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly extends cannotBeOrderedNegativeFP to include exponential functions (FEXP, FEXP2, FEXP10), which are always positive. The use of a switch statement is a good improvement for readability. I've pointed out a minor issue with some unreachable code that can be removed.
|
|
||
| llvm_unreachable("covered opcode switch"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gemini-code-assist can you read https://llvm.org/docs/CodingStandards.html and see if this suggestion still applies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, my suggestion still applies. I've reviewed the LLVM Coding Standards, and while they encourage the use of llvm_unreachable for code paths that should logically never be reached, they also implicitly advocate for clean code that avoids compiler warnings.
In this specific case, the switch statement's default case already includes a return false;. This means that from a compiler's perspective, the llvm_unreachable call immediately following the switch statement is syntactically unreachable. Compilers are typically smart enough to detect this and will issue a warning (e.g., -Wunreachable-code in Clang/GCC).
The purpose of llvm_unreachable is to assert a logical impossibility that the compiler might not otherwise deduce. When a default case already guarantees a return, the llvm_unreachable serves no practical purpose and only introduces a warning. Removing it aligns with the spirit of writing clean, warning-free code, which is a core tenet of good coding standards, including LLVM's.
Therefore, I recommend removing the llvm_unreachable line and the preceding blank line.
| llvm_unreachable("covered opcode switch"); | |
| } | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gemini-code-assist Give me high-level overview of things mentioned in https://llvm.org/docs/CodingStandards.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly! Here's a high-level overview of the key aspects covered in the LLVM Coding Standards:
- Readability and Maintainability: This is a core principle. Code should be easy to understand, even by someone unfamiliar with it. This includes clear naming, consistent formatting, and appropriate commenting.
- Formatting:
- Indentation: 2 spaces, no tabs.
- Braces: K&R style (opening brace on the same line as the statement, closing brace on its own line).
- Line Length: Generally 80 columns, with some exceptions for long strings or URLs.
- Whitespace: Use spaces around operators, after commas, etc.
- Naming Conventions:
- Classes/Structs:
CamelCase(e.g.,MyClassName). - Functions:
camelCase(e.g.,myFunctionName). - Variables:
camelCase(e.g.,myVariableName). - Macros/Enums:
ALL_CAPS_WITH_UNDERSCORES(e.g.,MY_MACRO).
- Classes/Structs:
- Comments:
- Explain why code does what it does, not just what it does.
- Use
///for Doxygen-style documentation comments for public APIs. - Use
//for internal comments.
- C++ Specific Guidelines:
- Classes: Prefer small, focused classes. Use
privatefor member variables. - Functions: Keep functions small and focused. Avoid long parameter lists.
- Smart Pointers: Use
std::unique_ptrandstd::shared_ptrfor memory management. - Error Handling: Use
llvm::Errorandllvm::Expectedfor robust error handling, rather than exceptions. constCorrectness: Useconstextensively for parameters, member functions, and variables to improve safety and clarity.overrideandfinal: Useoverridefor virtual functions andfinalwhere appropriate.
- Classes: Prefer small, focused classes. Use
- Performance: Be mindful of performance, especially in hot code paths. Avoid unnecessary allocations, copies, and complex data structures where simpler ones suffice.
- Testing: Encourage comprehensive unit tests for new features and bug fixes.
- Avoiding Warnings: Strive for warning-free code. Compilers are often right about potential issues.
- Portability: Write code that is portable across different platforms and compilers.
In essence, the LLVM Coding Standards emphasize clear, consistent, robust, and performant code, with a strong focus on C++ best practices and avoiding common pitfalls.
No description provided.