Cinder icon indicating copy to clipboard operation
Cinder copied to clipboard

Workaround for VC++ 14 bug with URLs with 260+ chars

Open benjaminbojko opened this issue 7 years ago • 4 comments

First pass at addressing #1979. This patch resolves the original issue we had at our studio with loading URLs with long query strings (e.g. this graphql query).

A couple of notes:

  • Since yesterday (03/01/2018) the bug is reported as resolved on the MS dev forums and will be included in the next VS compiler release. Because of that I limited the patch scope to _MSC_VER and not globally to CINDER_MSW. Thoughts?
  • The same issue of converting the URL string to fs::path in order to conveniently get the filename popped up in two places: DataSourceUrl and IStreamUrl. Currently I have nearly the same snippet in both locations, but am happy to consolidate if there are any suggestions for where that could/should live.
  • I followed @andrewfb's suggestion to attempt a simple filename retrieval using an rfind('/') . This works for our use-cases but I don't have a ton of insight into what other situations the DataSourceUrl::getFilePathHint() and StreamBase::getFileName() methods are used for. Any other suggestions are welcome, but I felt like a simple fix now is better than a beefier workaround later.

Happy to iterate over this.

benjaminbojko avatar Mar 02 '18 23:03 benjaminbojko

Sorry for the delay here; this looks good to me except for the <=1912. Specifically, I just tried this with the very recently released 2017 (1913) and it's still crashing. For now maybe we just say (>=1900) and then we'll revisit when the bug is actually fixed?

andrewfb avatar Mar 15 '18 03:03 andrewfb

As far as I understand from the linked issue, this bug will stay in std::experimental::filesystem and will only be fixed in std::filesystem (which afaik isn't released for msvc yet)

MikeGitb avatar Mar 15 '18 08:03 MikeGitb

Both good points. I tested this with VS 2015's native toolsets and they still have the same issue, so might as well do a catch-all for MSW at this point instead of limiting it to versions. New commit applies to all CINDER_MSW builds.

benjaminbojko avatar Mar 15 '18 17:03 benjaminbojko

Is this still relevant or has it been superseded by cinders move to VS2019 and std::filesystem?

MikeGitb avatar Feb 19 '20 19:02 MikeGitb