[PATCH] D76863: Fix SelectionDAG Graph Printing on Windows

[Reid Kleckner via Phabricator via llvm-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

Thanks, I think this looks good.

Do you need someone to push this? If so, let me know, I'll try to get to it.



================
Comment at: llvm/lib/Support/GraphWriter.cpp:87-88
+
+  for (std::string::iterator It = IllegalChars.begin(); It < IllegalChars.end();
+       ++It) {
+    std::replace(Filename.begin(), Filename.end(), *It, ReplacementChar);
----------------
This can be a range-based for.


================
Comment at: llvm/lib/Support/GraphWriter.cpp:101
+  std::string N = Name.str();
+  N = N.substr(0, std::min<std::size_t>(N.size(), 140));
+
----------------
justice_adams wrote:
> amccarth wrote:
> > If `sys::fs::createTemporaryFile` can return a too-long path here, then it could do so for anyone else, too.  Should the length limit be handled there?
> > 
> > Is 140 arbitrary, a guestimate, or based on some actual limit?
> @amccarth It appears to be arbitrary https://reviews.llvm.org/D3883
> 
> I'm open to moving the length limiting to 
> ```
> sys::fs::createTemporaryFile
> ```
> If that's preferred
I don't think that's necessary. I think most callers of `createTemporaryFile` supply a fixed prefix. The graph writer code here is special in that it is taking non-path-like user input and using that to form a path.


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

https://reviews.llvm.org/D76863