This is an archive of the discontinued LLVM Phabricator instance.

[clang][HeaderSearch] Make sure there are no backslashes in suggestedPath
ClosedPublic

Authored by kadircet on Apr 22 2019, 11:59 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

kadircet created this revision.Apr 22 2019, 11:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2019, 11:59 PM

This looks like a bug rather than a feature of HeaderSearch, shouldn't the slash conversion go there?

(It seems unlikely that diagnostics should suggest one thing but we insert another)

I had just looked at the method name and thought it was the right place( suggestPathToFileForDiagnostics since it says "path to file".)

But looking at the comments,

/// Suggest a path by which the specified file could be found, for
/// use in diagnostics to suggest a #include.

HeaderSearch API itself seems like a better place, I suppose the confusion comes from the fact that paths and the include-directives are
the same on unix-like systems.

Updating the patch

kadircet updated this revision to Diff 196192.Apr 23 2019, 12:43 AM
  • Fix bug in HeaderSearch instead of clangd
kadircet retitled this revision from [clangd] Make sure include path does not contain any backslashes on Windows to [clang][HeaderSearch] Make sure there are no backslashes in suggestedPath.Apr 23 2019, 12:43 AM
ormris added a subscriber: ormris.Apr 23 2019, 9:37 AM
sammccall accepted this revision.Apr 24 2019, 12:07 AM

Thanks!
Can you document this in HeaderSearch.h?

This revision is now accepted and ready to land.Apr 24 2019, 12:07 AM
kadircet updated this revision to Diff 196392.Apr 24 2019, 12:39 AM
  • Update documentation
sammccall added inline comments.Apr 24 2019, 12:42 AM
clang/include/clang/Lex/HeaderSearch.h
710 ↗(On Diff #196392)

I don't think this is specific enough - on windows, backslashes are valid (or at least accepted by clang).
I'd suggest specifically mentioning forward slashes.

kadircet updated this revision to Diff 196393.Apr 24 2019, 12:48 AM
  • Update comments to focus on forward slashes
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2019, 1:43 AM