[PATCH] D112758: [llvm] [Debuginfo] Debuginfod client library.

[Noah Shutty via Phabricator via llvm-commits]

noajshu added inline comments.


================
Comment at: llvm/include/llvm/Debuginfod/Debuginfod.h:42
+/// DEBUGINFOD_TIMEOUT environment variable, default is 90 seconds (90000 ms).
+long getDefaultDebuginfodTimeoutMS();
+
----------------
labath wrote:
> how about returning `std::chrono::milliseconds` instead?
Sure, I've updated this. By the way, do you think the `HTTPClient.setTimeout` should accept `std::chrono::milliseconds` as well? If so I can update D112751 and D112753. Otherwise, it just converts back to an integer using `.count()` before passing to `setTimeout`.


================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:66
+      to_integer(StringRef(DebuginfodTimeoutEnv).trim(), TimeoutMS, 10))
+    return TimeoutMS * 1000;
+
----------------
dblaikie wrote:
> I guess TimeoutMS is actually a timeout in seconds? (or it's in ms and shouldn't be multiplied by 1000)?
Good catch. I converted to use `std::chrono::milliseconds` as @labath suggested and also removed the `MS` suffix here.


================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:148
+  Client.setTimeout(Timeout);
+  for (const StringRef &ServerUrl : DebuginfodUrls) {
+    SmallString<64> ArtifactUrl;
----------------
labath wrote:
> since we're passing them by value, I'd say we can also iterate over them in this way...
ok thanks, this makes sense!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112758/new/

https://reviews.llvm.org/D112758