[Lldb-commits] [lldb] r263333 - Let's not convert from UINT32_MAX to the std::numeric_limits version.
Jim Ingham via lldb-commits
lldb-commits at lists.llvm.org
Tue Mar 15 15:16:19 PDT 2016
> On Mar 15, 2016, at 3:00 PM, Zachary Turner <zturner at google.com> wrote:
>
> I agree that they are pretty much equivalent, I guess the main advantage I see is that you don't have to think about what type something is when assigning it an "invalid" value. For example, LLDB_INVALID_THREAD_ID could just as easily be std::numeric_limits<lldb::tid_t>::min(), and then you don't have to worry about whether it's signed, or unsigned, or 32 bits, or 64-bits.
I'd be fine with this at the place where LLDB_INVALID_THREAD_ID was defined if that somehow made things more better. But std::numeric_limits<lldb::tid_t>::min() is not very expressive of the fact that you're testing for an invalid thread ID, so I wouldn't want to use that as the test in ordinary code. I bet many of the uses of UINT32_MAX are places where we should have invented (or did invent but didn't use) a more significant name. Cleaning that up will not be made easier by replacing UINT32_MAX with the limits version.
Jim
>
> I guess put another way, if they're equivalent then we should prefer the C++ ones if for no other reason than that it makes porting refactoring, porting, and other similar tasks easier in the future (even if it seems unlikely and the benefit is mostly theoretical).
>
> Anyway, doesn't really matter, I just thought it strange to see C++ code ported to C, as usually it happens the other way around.
>
> On Tue, Mar 15, 2016 at 2:37 PM Jim Ingham <jingham at apple.com> wrote:
> It doesn't look like there is any advantage to the numeric_limits if you already know the type you are getting the max or min of. The template would be nice for doing more generic programming, but we're never doing anything fancier than "What is the max of a uint32_t". It doesn't look like there is any advantage to the numeric_limits if you already know the type you are getting the max or min of.
>
> In general we tend not to use raw UINT32_MAX and the like, but semantically significant equivalents like LLDB_INVALID_THREAD_ID, etc. Some of the uses of UINT32_MAX here should have been LLDB_INVALID_FILE_INDEX32, for instance, though not all. At some point we should go clean up uses of *_MAX and use more meaningful defines where appropriate. But I can't see that there is any value to replacing the current _MAX usage with the much more verbose numeric_limits invocations.
>
> Jim
>
>
> > On Mar 15, 2016, at 2:20 PM, Zachary Turner <zturner at google.com> wrote:
> >
> > Is the stdint version better somehow? I thought C++ numeric_limits were actually preferred over the C macros.
> >
> > On Mon, Mar 14, 2016 at 7:59 AM Adrian McCarthy via lldb-commits <lldb-commits at lists.llvm.org> wrote:
> > If we're favoring the <stdint.h> macros over the <limits> functions, then perhaps update the #includes?
> >
> > On Fri, Mar 11, 2016 at 7:33 PM, Jim Ingham via lldb-commits <lldb-commits at lists.llvm.org> wrote:
> > Author: jingham
> > Date: Fri Mar 11 21:33:36 2016
> > New Revision: 263333
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=263333&view=rev
> > Log:
> > Let's not convert from UINT32_MAX to the std::numeric_limits version.
> >
> > Modified:
> > lldb/trunk/source/Core/DataEncoder.cpp
> > lldb/trunk/source/Core/Disassembler.cpp
> > lldb/trunk/source/Core/FileSpecList.cpp
> > lldb/trunk/source/Core/SearchFilter.cpp
> >
> > Modified: lldb/trunk/source/Core/DataEncoder.cpp
> > URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/DataEncoder.cpp?rev=263333&r1=263332&r2=263333&view=diff
> > ==============================================================================
> > --- lldb/trunk/source/Core/DataEncoder.cpp (original)
> > +++ lldb/trunk/source/Core/DataEncoder.cpp Fri Mar 11 21:33:36 2016
> > @@ -233,7 +233,7 @@ DataEncoder::PutU8 (uint32_t offset, uin
> > m_start[offset] = value;
> > return offset + 1;
> > }
> > - return std::numeric_limits<uint32_t>::max();
> > + return UINT32_MAX;
> > }
> >
> > uint32_t
> > @@ -248,7 +248,7 @@ DataEncoder::PutU16 (uint32_t offset, ui
> >
> > return offset + sizeof (value);
> > }
> > - return std::numeric_limits<uint32_t>::max();
> > + return UINT32_MAX;
> > }
> >
> > uint32_t
> > @@ -263,7 +263,7 @@ DataEncoder::PutU32 (uint32_t offset, ui
> >
> > return offset + sizeof (value);
> > }
> > - return std::numeric_limits<uint32_t>::max();
> > + return UINT32_MAX;
> > }
> >
> > uint32_t
> > @@ -278,7 +278,7 @@ DataEncoder::PutU64 (uint32_t offset, ui
> >
> > return offset + sizeof (value);
> > }
> > - return std::numeric_limits<uint32_t>::max();
> > + return UINT32_MAX;
> > }
> >
> > //----------------------------------------------------------------------
> > @@ -304,7 +304,7 @@ DataEncoder::PutMaxU64 (uint32_t offset,
> > assert(!"GetMax64 unhandled case!");
> > break;
> > }
> > - return std::numeric_limits<uint32_t>::max();
> > + return UINT32_MAX;
> > }
> >
> > uint32_t
> > @@ -318,7 +318,7 @@ DataEncoder::PutData (uint32_t offset, c
> > memcpy (m_start + offset, src, src_len);
> > return offset + src_len;
> > }
> > - return std::numeric_limits<uint32_t>::max();
> > + return UINT32_MAX;
> > }
> >
> > uint32_t
> > @@ -332,5 +332,5 @@ DataEncoder::PutCString (uint32_t offset
> > {
> > if (cstr != nullptr)
> > return PutData (offset, cstr, strlen(cstr) + 1);
> > - return std::numeric_limits<uint32_t>::max();
> > + return UINT32_MAX;
> > }
> >
> > Modified: lldb/trunk/source/Core/Disassembler.cpp
> > URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/Disassembler.cpp?rev=263333&r1=263332&r2=263333&view=diff
> > ==============================================================================
> > --- lldb/trunk/source/Core/Disassembler.cpp (original)
> > +++ lldb/trunk/source/Core/Disassembler.cpp Fri Mar 11 21:33:36 2016
> > @@ -1036,7 +1036,7 @@ InstructionList::GetIndexOfNextBranchIns
> > {
> > size_t num_instructions = m_instructions.size();
> >
> > - uint32_t next_branch = std::numeric_limits<uint32_t>::max();
> > + uint32_t next_branch = UINT32_MAX;
> > size_t i;
> > for (i = start; i < num_instructions; i++)
> > {
> > @@ -1053,7 +1053,7 @@ InstructionList::GetIndexOfNextBranchIns
> > if (target.GetArchitecture().GetTriple().getArch() == llvm::Triple::hexagon)
> > {
> > // If we didn't find a branch, find the last packet start.
> > - if (next_branch == std::numeric_limits<uint32_t>::max())
> > + if (next_branch == UINT32_MAX)
> > {
> > i = num_instructions - 1;
> > }
> > @@ -1086,7 +1086,7 @@ InstructionList::GetIndexOfNextBranchIns
> > }
> > }
> >
> > - if (next_branch == std::numeric_limits<uint32_t>::max())
> > + if (next_branch == UINT32_MAX)
> > {
> > // We couldn't find the previous packet, so return start
> > next_branch = start;
> > @@ -1099,7 +1099,7 @@ uint32_t
> > InstructionList::GetIndexOfInstructionAtAddress (const Address &address)
> > {
> > size_t num_instructions = m_instructions.size();
> > - uint32_t index = std::numeric_limits<uint32_t>::max();
> > + uint32_t index = UINT32_MAX;
> > for (size_t i = 0; i < num_instructions; i++)
> > {
> > if (m_instructions[i]->GetAddress() == address)
> > @@ -1152,7 +1152,7 @@ Disassembler::ParseInstructions (const E
> > m_arch.GetByteOrder(),
> > m_arch.GetAddressByteSize());
> > const bool data_from_file = load_addr == LLDB_INVALID_ADDRESS;
> > - return DecodeInstructions(range.GetBaseAddress(), data, 0, std::numeric_limits<uint32_t>::max(), false,
> > + return DecodeInstructions(range.GetBaseAddress(), data, 0, UINT32_MAX, false,
> > data_from_file);
> > }
> > else if (error_strm_ptr)
> >
> > Modified: lldb/trunk/source/Core/FileSpecList.cpp
> > URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/FileSpecList.cpp?rev=263333&r1=263332&r2=263333&view=diff
> > ==============================================================================
> > --- lldb/trunk/source/Core/FileSpecList.cpp (original)
> > +++ lldb/trunk/source/Core/FileSpecList.cpp Fri Mar 11 21:33:36 2016
> > @@ -125,7 +125,7 @@ FileSpecList::FindFileIndex (size_t star
> > }
> >
> > // We didn't find the file, return an invalid index
> > - return std::numeric_limits<uint32_t>::max();
> > + return UINT32_MAX;
> > }
> >
> > //------------------------------------------------------------------
> >
> > Modified: lldb/trunk/source/Core/SearchFilter.cpp
> > URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/SearchFilter.cpp?rev=263333&r1=263332&r2=263333&view=diff
> > ==============================================================================
> > --- lldb/trunk/source/Core/SearchFilter.cpp (original)
> > +++ lldb/trunk/source/Core/SearchFilter.cpp Fri Mar 11 21:33:36 2016
> > @@ -266,7 +266,10 @@ SearchFilter::DoFunctionIteration (Funct
> > bool
> > SearchFilterForUnconstrainedSearches::ModulePasses (const FileSpec &module_spec)
> > {
> > - return (!m_target_sp->ModuleIsExcludedForUnconstrainedSearches(module_spec));
> > + if (m_target_sp->ModuleIsExcludedForUnconstrainedSearches (module_spec))
> > + return false;
> > + else
> > + return true;
> > }
> >
> > bool
> > @@ -445,7 +448,7 @@ SearchFilterByModuleList::ModulePasses (
> > return true;
> >
> > if (module_sp &&
> > - m_module_spec_list.FindFileIndex(0, module_sp->GetFileSpec(), false) != std::numeric_limits<uint32_t>::max())
> > + m_module_spec_list.FindFileIndex(0, module_sp->GetFileSpec(), false) != UINT32_MAX)
> > return true;
> > else
> > return false;
> > @@ -457,7 +460,7 @@ SearchFilterByModuleList::ModulePasses (
> > if (m_module_spec_list.GetSize() == 0)
> > return true;
> >
> > - if (m_module_spec_list.FindFileIndex(0, spec, true) != std::numeric_limits<uint32_t>::max())
> > + if (m_module_spec_list.FindFileIndex(0, spec, true) != UINT32_MAX)
> > return true;
> > else
> > return false;
> > @@ -506,7 +509,7 @@ SearchFilterByModuleList::Search (Search
> > for (size_t i = 0; i < num_modules; i++)
> > {
> > Module* module = target_modules.GetModulePointerAtIndexUnlocked(i);
> > - if (m_module_spec_list.FindFileIndex(0, module->GetFileSpec(), false) != std::numeric_limits<uint32_t>::max())
> > + if (m_module_spec_list.FindFileIndex(0, module->GetFileSpec(), false) != UINT32_MAX)
> > {
> > SymbolContext matchingContext(m_target_sp, module->shared_from_this());
> > Searcher::CallbackReturn shouldContinue;
> > @@ -613,13 +616,13 @@ SearchFilterByModuleListAndCU::AddressPa
> > bool
> > SearchFilterByModuleListAndCU::CompUnitPasses (FileSpec &fileSpec)
> > {
> > - return m_cu_spec_list.FindFileIndex(0, fileSpec, false) != std::numeric_limits<uint32_t>::max();
> > + return m_cu_spec_list.FindFileIndex(0, fileSpec, false) != UINT32_MAX;
> > }
> >
> > bool
> > SearchFilterByModuleListAndCU::CompUnitPasses (CompileUnit &compUnit)
> > {
> > - bool in_cu_list = m_cu_spec_list.FindFileIndex(0, compUnit, false) != std::numeric_limits<uint32_t>::max();
> > + bool in_cu_list = m_cu_spec_list.FindFileIndex(0, compUnit, false) != UINT32_MAX;
> > if (in_cu_list)
> > {
> > ModuleSP module_sp(compUnit.GetModule());
> > @@ -662,7 +665,7 @@ SearchFilterByModuleListAndCU::Search (S
> > {
> > lldb::ModuleSP module_sp = target_images.GetModuleAtIndexUnlocked(i);
> > if (no_modules_in_filter ||
> > - m_module_spec_list.FindFileIndex(0, module_sp->GetFileSpec(), false) != std::numeric_limits<uint32_t>::max())
> > + m_module_spec_list.FindFileIndex(0, module_sp->GetFileSpec(), false) != UINT32_MAX)
> > {
> > SymbolContext matchingContext(m_target_sp, module_sp);
> > Searcher::CallbackReturn shouldContinue;
> > @@ -682,8 +685,7 @@ SearchFilterByModuleListAndCU::Search (S
> > matchingContext.comp_unit = cu_sp.get();
> > if (matchingContext.comp_unit)
> > {
> > - if (m_cu_spec_list.FindFileIndex(0, *matchingContext.comp_unit, false) !=
> > - std::numeric_limits<uint32_t>::max())
> > + if (m_cu_spec_list.FindFileIndex(0, *matchingContext.comp_unit, false) != UINT32_MAX)
> > {
> > shouldContinue = DoCUIteration(module_sp, matchingContext, searcher);
> > if (shouldContinue == Searcher::eCallbackReturnStop)
> >
> >
> > _______________________________________________
> > lldb-commits mailing list
> > lldb-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
> >
> > _______________________________________________
> > lldb-commits mailing list
> > lldb-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
More information about the lldb-commits
mailing list