This is an archive of the discontinued LLVM Phabricator instance.

[lldb] More tests for DumpDataExtractor
ClosedPublic

Authored by DavidSpickett on Apr 30 2021, 6:47 AM.

Details

Summary
  • Using a base address or skipping it with LLDB_INVALID_ADDRESS
  • Using a data offset, which does not effect the printed addresses
  • Not providing an output stream
  • Formatting a double sized HexFloat
  • Formatting over multiple lines

Since address printing now has its own test,
I've removed the base address from all the format
type tests.

The multi line tests still use a base address to check that
it's incremented correctly for each new line.

Diff Detail

Event Timeline

DavidSpickett requested review of this revision.Apr 30 2021, 6:47 AM
DavidSpickett created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2021, 6:47 AM
teemperor accepted this revision.Apr 30 2021, 7:27 AM

The_underscore_style_for_functions isn't LLDB (or LLVM) code style. I missed that in the first review. So just rename the utility functions to TestDumpMultiline and so on and this LGTM. I'm just going to accept this as renaming the variables shouldn't need another review.

This revision is now accepted and ready to land.Apr 30 2021, 7:27 AM

Change function names to match coding style.

This revision was landed with ongoing or failed builds.Apr 30 2021, 8:16 AM
This revision was automatically updated to reflect the committed changes.

The_underscore_style_for_functions isn't LLDB (or LLVM) code style. I missed that in the first review. So just rename the utility functions to TestDumpMultiline and so on and this LGTM. I'm just going to accept this as renaming the variables shouldn't need another review.

I take it that LLDB is also following the variable naming style then? At least for new code, older code like what this is testing doesn't.

The_underscore_style_for_functions isn't LLDB (or LLVM) code style. I missed that in the first review. So just rename the utility functions to TestDumpMultiline and so on and this LGTM. I'm just going to accept this as renaming the variables shouldn't need another review.

I take it that LLDB is also following the variable naming style then? At least for new code, older code like what this is testing doesn't.

We're still following the LLDB style of using_underscores_for_variables and the m_ prefix for member variables.

In a perfect world we would just reformat everything to LLVM code style, but sadly many of us are still anchored in this reality by things such as downstream forks.

lldb/unittests/Core/DumpDataExtractorTest.cpp
40

I think you missed the capitalisation here and in the function above :)

DavidSpickett added inline comments.Apr 30 2021, 8:37 AM
lldb/unittests/Core/DumpDataExtractorTest.cpp
40

Doh. In fact it's the opposite, TestDump... should be testDump.... at least according to llvm style.

https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

Does lldb differ there too?

https://lldb.llvm.org/resources/contributing.html

(maybe I can contribute some writeup of these differences)

DavidSpickett added inline comments.Apr 30 2021, 8:45 AM
lldb/unittests/Core/DumpDataExtractorTest.cpp
40

Well now I look at some random files, yes it differs. I thought maybe only member functions were FooBar but that applies all over. I'll fix it.

Jepp, they are capitalized :)

FWIW, maybe we should see if we can fix this at the next US dev meeting (which hopefully happens). git has by now a filter for mass-refactors so the only problem is getting everyone to on board with breaking the whole code base and make them rewrite the internal patches. Fun!

I would resist this change. It's unnecessarily disruptive, would again break git archeology, and really have no significant benefit. I also think the lldb conventions for naming things are much clearer than the llvm ones. Knowing that something is a ivar by looking at the name is a real timesaver, especially for people new to the code. Being able to tell local variables from other entities by looking also makes reading code much easier. Etc...

I would be willing to discuss reformatting the llvm codebase to follow the lldb conventions, however...

Jim

FWIW, I think the idea was for LLVM to actually move to LLDB's function naming, so our life will soon(TM) be easier when/if that happens.

I also agree that the CapitalizedVariableNames in Clang have some difficulties (especially when they conflict with class names), but if we start this discussion at the dev meeting then I think everyone can say what they think.

shafik added a subscriber: shafik.Apr 30 2021, 11:38 AM

+1

I would resist this change. It's unnecessarily disruptive, would again break git archeology, and really have no significant benefit. I also think the lldb conventions for naming things are much clearer than the llvm ones. Knowing that something is a ivar by looking at the name is a real timesaver, especially for people new to the code. Being able to tell local variables from other entities by looking also makes reading code much easier. Etc...

I would be willing to discuss reformatting the llvm codebase to follow the lldb conventions, however...

Jim