Fixes PR38940.
Details
Diff Detail
Event Timeline
COFF/Driver.cpp | ||
---|---|---|
827 | Move this comment before this function definition to make it a function comment. | |
834 | I think you can omit llvm::. | |
843 | Instead of handling a... fragment all at once, you can read one character at a time, and I believe that's slightly simpler. E.g. while (!AltPath.empty()) { if (!AltPath.startswith("%") Buf.append(AltPath[0]); else if (AltPath.startswith("%_pdb%") .... else if (AltPath.startswith("%_ext%") .... else .... } |
Thanks!
COFF/Driver.cpp | ||
---|---|---|
843 | I'm not sure it's much simpler, because the else needs to skip until the next % (for printing the warning, and for correctness). Here's how it looks with the .... filled in (maybe I didn't fill in the .... as you pictured though): static void parsePDBAltPath(StringRef AltPath) { SmallString<128> Buf; StringRef PDBBasename = sys::path::filename(Config->PDBPath, sys::path::Style::windows); StringRef BinaryExtension = sys::path::extension(Config->OutputFile, sys::path::Style::windows); if (!BinaryExtension.empty()) BinaryExtension = BinaryExtension.substr(1); // %_EXT% does not include '.'. while (!AltPath.empty()) { if (!AltPath.startswith("%")) { Buf.append(1, AltPath[0]); AltPath = AltPath.drop_front(1); } else if (AltPath.startswith_lower("%_pdb%")) { Buf.append(PDBBasename); AltPath = AltPath.drop_front(strlen("%_pdb%")); } else if (AltPath.startswith_lower("%_ext%")) { Buf.append(BinaryExtension); AltPath = AltPath.drop_front(strlen("%_ext%")); } else { size_t P = AltPath.find('%', 1); if (P == StringRef::npos) { Buf.append(AltPath); break; } StringRef Var = AltPath.substr(0, P + 1); warn("only %_PDB% and %_EXT% supported in /pdbaltpath:, keeping " + Var + " as literal"); Buf.append(Var); AltPath = AltPath.drop_front(Var.size()); } } Config->PDBAltPath = Buf; } or, slightly different: static void parsePDBAltPath(StringRef AltPath) { SmallString<128> Buf; StringRef PDBBasename = sys::path::filename(Config->PDBPath, sys::path::Style::windows); StringRef BinaryExtension = sys::path::extension(Config->OutputFile, sys::path::Style::windows); if (!BinaryExtension.empty()) BinaryExtension = BinaryExtension.substr(1); // %_EXT% does not include '.'. while (!AltPath.empty()) { if (!AltPath.startswith("%")) { Buf.append(1, AltPath[0]); AltPath = AltPath.drop_front(1); } else { size_t P = AltPath.find('%', 1); if (P == StringRef::npos) { Buf.append(AltPath); break; } StringRef Var = AltPath.substr(0, P + 1); if (Var.equals_lower("%_pdb%")) Buf.append(PDBBasename); else if (Var.equals_lower("%_ext%")) Buf.append(BinaryExtension); else { warn("only %_PDB% and %_EXT% supported in /pdbaltpath:, keeping " + Var + " as literal"); Buf.append(Var); } AltPath = AltPath.drop_front(Var.size()); } } Config->PDBAltPath = Buf; } Here's the algorithm in the patch with drop_front: static void parsePDBAltPath(StringRef AltPath) { SmallString<128> Buf; StringRef PDBBasename = sys::path::filename(Config->PDBPath, sys::path::Style::windows); StringRef BinaryExtension = sys::path::extension(Config->OutputFile, sys::path::Style::windows); if (!BinaryExtension.empty()) BinaryExtension = BinaryExtension.substr(1); // %_EXT% does not include '.'. size_t Cursor = 0; while (Cursor < AltPath.size()) { size_t FirstMark, SecondMark; if ((FirstMark = AltPath.find('%', Cursor)) == StringRef::npos || (SecondMark = AltPath.find('%', FirstMark + 1)) == StringRef::npos) { Buf.append(AltPath.substr(Cursor)); break; } Buf.append(AltPath.substr(Cursor, FirstMark - Cursor)); StringRef Var = AltPath.substr(FirstMark, SecondMark - FirstMark + 1); if (Var.equals_lower("%_pdb%")) Buf.append(PDBBasename); else if (Var.equals_lower("%_ext%")) Buf.append(BinaryExtension); else { warn("only %_PDB% and %_EXT% supported in /pdbaltpath:, keeping " + Var + " as literal"); Buf.append(Var); } Cursor = SecondMark + 1; } Config->PDBAltPath = Buf; } Take your pick :-) |
Hmm, okay, it seems all the choices are not that different, so I'm fine with your original one. Thank you for writing all the pieces of code by the way! :)
LGTM
Move this comment before this function definition to make it a function comment.