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

[NFC] Rename variable to better document usage. #71973

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

stephenpeckham
Copy link
Contributor

Change variable DisassembleAsData to DisassembleELFAsData so that its name better matches its usage.

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 10, 2023

@llvm/pr-subscribers-llvm-binary-utilities

Author: None (stephenpeckham)

Changes

Change variable DisassembleAsData to DisassembleELFAsData so that its name better matches its usage.


Full diff: https://github.com/llvm/llvm-project/pull/71973.diff

1 Files Affected:

  • (modified) llvm/tools/llvm-objdump/llvm-objdump.cpp (+5-5)
diff --git a/llvm/tools/llvm-objdump/llvm-objdump.cpp b/llvm/tools/llvm-objdump/llvm-objdump.cpp
index a112c50bf7f2715..e5b8e014392c54c 100644
--- a/llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ b/llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -1780,19 +1780,19 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
       // inside functions, for which STT_FUNC would be inaccurate.
       //
       // So here, we spot whether there's any non-data symbol present at all,
-      // and only set the DisassembleAsData flag if there isn't. Also, we use
+      // and only set the DisassembleELFAsData flag if there isn't. Also, we use
       // this distinction to inform the decision of which symbol to print at
       // the head of the section, so that if we're printing code, we print a
       // code-related symbol name to go with it.
-      bool DisassembleAsData = false;
+      bool DisassembleELFAsData = false;
       size_t DisplaySymIndex = SymbolsHere.size() - 1;
       if (Obj.isELF() && !DisassembleAll && Section.isText()) {
-        DisassembleAsData = true; // unless we find a code symbol below
+        DisassembleELFAsData = true; // unless we find a code symbol below
 
         for (size_t i = 0; i < SymbolsHere.size(); ++i) {
           uint8_t SymTy = SymbolsHere[i].Type;
           if (SymTy != ELF::STT_OBJECT && SymTy != ELF::STT_COMMON) {
-            DisassembleAsData = false;
+            DisassembleELFAsData = false;
             DisplaySymIndex = i;
           }
         }
@@ -1943,7 +1943,7 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
       if (SectionAddr < StartAddress)
         Index = std::max<uint64_t>(Index, StartAddress - SectionAddr);
 
-      if (DisassembleAsData) {
+      if (DisassembleELFAsData) {
         dumpELFData(SectionAddr, Index, End, Bytes);
         Index = End;
         continue;

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming is hard :)

I feel like it may be a mistake to rename this variable. Although the code is currently implemented for ELF only, I don't see why it couldn't work for other formats too, if someone were to take the time to implement it, as the concept of data versus text is pretty general. Is there a reason to think that the current name is causing confusion?

@stephenpeckham
Copy link
Contributor Author

Naming is hard :)

We have DumpARMELFData as a counterexample. And dumpELFData() has nothing to do with ELF. It's just dumping section data and could be used for any object file format.

I don't have strong feelings about this change. I'm happy to delete the PR.

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point. How about DisassembleAsELFData?

(I think that reads slightly better).

LGTM with that.

@stephenpeckham stephenpeckham merged commit 506c0fa into llvm:main Nov 15, 2023
3 checks passed
@stephenpeckham stephenpeckham deleted the peckham/disassembleelfasdata branch November 15, 2023 19:42
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
Change variable DisassembleAsData to DisassembleAsELFData so that its
name better matches its usage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants