[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

ayermolo added a comment. Found another instance: https://reviews.llvm.org/D141624 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139955/new/ https://reviews.llvm.org/D139955 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

ayermolo added a comment. In D139955#4047011 , @Michael137 wrote: > Ah it's just a typoed format (and lack of curly braces around one of the > format specifiers) > > Fixed in https://reviews.llvm.org/rGade3f1ccd807 Thanks, for a fix. Sorry missed it.

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

Michael137 added a comment. Ah I think it's just a lack of curly braces around one of the format specifiers. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139955/new/ https://reviews.llvm.org/D139955 ___

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

Michael137 added a comment. FYI, this is causing buildbot failures: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/49913/execution/node/74/log/ Currently checking what's wrong. /Users/buildslave/jenkins/workspace/lldb-cmake@2/llvm-project/lldb/test/API/lang/cpp/accelerator-table -p

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

ayermolo added a comment. Thanks for reviewing, really appreciate it! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139955/new/ https://reviews.llvm.org/D139955 ___ lldb-commits mailing list lldb-commits

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe262b8f48af9: [LLDB] Change formatting to use llvm::formatv (authored by ayermolo). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

ayermolo updated this revision to Diff 487512. ayermolo added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139955/new/ https://reviews.llvm.org/D139955 Files: lldb/include/lldb/Core/Module.h lldb/include/lldb/Utility/Status.

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. LGTM too Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139955/new/ https://reviews.llvm.org/D139955 ___

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

labath accepted this revision. labath added a comment. I'm sorry for the delay, I was out for the holidays. Looks now, thanks for your patience. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139955/new/ https://reviews.llvm.org/D139955 __

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

ayermolo updated this revision to Diff 486719. ayermolo added a comment. removed + Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139955/new/ https://reviews.llvm.org/D139955 Files: lldb/include/lldb/Core/Module.h lldb/include/lldb/Utility/Stat

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

JDevlieghere added a comment. This looks good to me if we drop the explicit `+` for right alignment: it's the default and other places in LLDB (and LLVM, at least as far as I'm aware) don't include this unless there's ambiguity. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

ayermolo added a comment. @labath ping. Is there anything else I need to do? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139955/new/ https://reviews.llvm.org/D139955 ___ lldb-commits mailing list lldb-

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

ayermolo added inline comments. Comment at: lldb/include/lldb/Utility/Status.h:65 + template + explicit Status(const char *format, Args &&...args) { +SetErrorToGenericError(); labath wrote: > I don't think you've converted all the callers of this one. Give

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

ayermolo updated this revision to Diff 484920. ayermolo marked 11 inline comments as done. ayermolo added a comment. addressing comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139955/new/ https://reviews.llvm.org/D139955 Files: lldb/inclu

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

labath added a comment. This is looking much better -- and focused. I still have comments though. :) Comment at: lldb/include/lldb/Core/Module.h:833 - void LogMessageVerboseBacktrace(Log *log, const char *format, ...) - __attribute__((format(printf, 3, 4))); + void Log

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

ayermolo updated this revision to Diff 484382. ayermolo added a comment. missed couple of Status uses. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139955/new/ https://reviews.llvm.org/D139955 Files: lldb/include/lldb/Core/Module.h lldb/inclu

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

ayermolo updated this revision to Diff 484261. ayermolo added a comment. removed include that snuck in. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139955/new/ https://reviews.llvm.org/D139955 Files: lldb/include/lldb/Core/Module.h lldb/incl

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

ayermolo updated this revision to Diff 484259. ayermolo added a comment. Herald added subscribers: MaskRay, emaste. pulled a trigger and changed all the call sites. I think less confusing then having two sets of APIs. One with printf symantics another with formatv. Repository: rG LLVM Github

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

ayermolo marked 2 inline comments as done. ayermolo added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:11-58 +#include "AppleDWARFIndex.h" +#include "DWARFASTParser.h" +#include "DWARFASTParserClang.h" +#include "DWARFCompileUnit.h" +#incl

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

ayermolo added a comment. In D139955#3999521 , @labath wrote: > In D139955#3999381 , @ayermolo > wrote: > >> I tried to modify all the places wehre getOffset() is used. Which will later >> return 64bit instead o

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

labath added a comment. In D139955#3999381 , @ayermolo wrote: > I tried to modify all the places wehre getOffset() is used. Which will later > return 64bit instead of 32 bit. > > Sorry I guess there was miss communication. > I am not quite clear in what

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

ayermolo added a comment. I tried to modify all the places wehre getOffset() is used. Which will later return 64bit instead of 32 bit. Sorry I guess there was miss communication. I am not quite clear in what you are suggesting. The way I did it we now have two distinct interfaces. One for old wa

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

labath added a comment. Yeah, I'm afraid this is all my fault. I suggested going in this direction, but this isn't exactly how I thought it would turn out. I was expecting that the inferface would be something along the lines of what Jonas posted in his comment. And I was not expecting that you

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

ayermolo added a comment. In D139955#3992766 , @JDevlieghere wrote: > I don't really understand the motivation. Can you elaborate on why "enabling > 64 bit support" requires this change? I definitely prefer the `formatv` > approach, but wouldn't the PR

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

JDevlieghere requested changes to this revision. JDevlieghere added a comment. This revision now requires changes to proceed. I don't really understand the motivation. Can you elaborate on why "enabling 64 bit support" requires this change? I definitely prefer the `formatv` approach, but wouldn'

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

ayermolo created this revision. Herald added subscribers: hoy, modimo, wenlei, arphaman. Herald added a reviewer: shafik. Herald added a project: All. ayermolo requested review of this revision. Herald added a reviewer: jdoerfert. Herald added subscribers: lldb-commits, sstefan1. Herald added a pro