Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SerializeLoc: serialize basic decl source location information to .swiftsourceinfo file #27464

Merged
merged 17 commits into from Oct 10, 2019

Conversation

@nkcsgexi
Copy link
Contributor

commented Oct 1, 2019

After setting up the .swiftsourceinfo file, this patch starts to actually serialize and de-serialize source locations for declaration. The binary format of .swiftsourceinfo currently contains these three records:

BasicDeclLocs: a hash table mapping from a USR ID to a list of basic source locations. The USR id could be retrieved from the following DeclUSRs record using an actual decl USR. The basic source locations include a file ID and the results from Decl::getLoc(), ValueDecl::getNameLoc(), Decl::getStartLoc() and Decl::getEndLoc(). The file ID could be used to retrieve the actual file name from the following SourceFilePaths record. Each location is encoded as a line:column pair.

DeclUSRS: a hash table mapping from USR to a USR ID used by location records.

SourceFilePaths: a hash table mapping from a file ID to actual file name.

BasicDeclLocs should be sufficient for most diagnostic cases. If additional source locations are needed, we could always add new source location records without breaking the backward compatibility. When de-serializing the source location from a module-imported decl, we calculate its USR, retrieve the USR ID from the DeclUSRS record, and use the USR ID to look up the basic location list in the BasicDeclLocs record.

For more details about .swiftsourceinfo file: https://forums.swift.org/t/proposal-emitting-source-information-file-during-compilation

rdar://56167685

@nkcsgexi nkcsgexi changed the title Deserialize source info [WIP] deserialize source info Oct 1, 2019
@nkcsgexi nkcsgexi changed the title [WIP] deserialize source info [WIP] serialize/deserialize source info Oct 1, 2019
@nkcsgexi nkcsgexi force-pushed the nkcsgexi:deserialize-source-info branch 3 times, most recently from a20e98f to d7eccd3 Oct 1, 2019
// SECOND: Inputs/def_comments.swift:28:13: Var/decl_var_2 RawComment=none BriefComment=none DocCommentAsXML=none
// SECOND: Inputs/def_comments.swift:28:25: Var/decl_var_3 RawComment=none BriefComment=none DocCommentAsXML=none
// SECOND: Inputs/def_comments.swift:28:25: Var/decl_var_3 RawComment=none BriefComment=none DocCommentAsXML=none
// SECOND: NonExistingSource.swift:100000:13: Func/functionAfterPoundSourceLoc

This comment has been minimized.

Copy link
@nkcsgexi

nkcsgexi Oct 3, 2019

Author Contributor

@rintaro as you have suggested, a test case for #sourceLocation is added here.

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 11, 2019

Member

Should this include a test that it's made absolute? Even if that's not important behavior, we probably still want to know if it changes.

@nkcsgexi nkcsgexi force-pushed the nkcsgexi:deserialize-source-info branch from fd391a0 to 91f0804 Oct 3, 2019
@nkcsgexi

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2019

@swift-ci please smoke test

@nkcsgexi nkcsgexi force-pushed the nkcsgexi:deserialize-source-info branch from effa580 to 776cd02 Oct 4, 2019
@nkcsgexi nkcsgexi changed the title [WIP] serialize/deserialize source info SerializeLoc: serialize basic decl source location information to .swiftsourceinfo file Oct 4, 2019
@nkcsgexi

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2019

@swift-ci please smoke test

@nkcsgexi

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2019

@swift-ci Please test compiler performance

@nkcsgexi

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2019

@swift-ci Please Build Toolchain macOS Platform

struct BasicDeclLocs {
StringRef SourceFilePath;
Optional<LineColumn> Loc;
Optional<LineColumn> NameLoc;

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 4, 2019

Member

I don't think we have any decls where Loc and NameLoc are different. Is this just to account for decls without names?

This comment has been minimized.

Copy link
@nkcsgexi

nkcsgexi Oct 5, 2019

Author Contributor

We have separate APIs getLoc() and getNameLoc() for ValueDecl, so they could potentially be different. IMO, we shouldn't hardcode that they are usually identical to the format.

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 5, 2019

Member

I'll flip it around: there are zero cases where getLocFromSource() does not return getNameLoc() if there is a getNameLoc(), and we could put that in the AST verifier.

Copy link
Member

left a comment

A storm of comments on just the first commit, sorry.

@@ -50,6 +51,10 @@ bool printAccessorUSR(const AbstractStorageDecl *D, AccessorKind AccKind,
/// \returns true if it failed, false on success.
bool printExtensionUSR(const ExtensionDecl *ED, raw_ostream &OS);

/// Prints out the Decl USRs suitable for keys .swiftdoc and .swiftsourceinfo files.
/// \returns true if it failed, false on success.
bool printDeclUSRForModuleDoc(const Decl *D, raw_ostream &OS);

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 5, 2019

Member

Nitpick: I don't think it makes sense to have doc stuff be the special case here. Can we rename printDeclUSR to printValueDeclUSR and have this just be printDeclUSR, or printAnyDeclUSR? (And filter out implicit decls on the caller side.)

std::pair<std::unique_ptr<llvm::MemoryBuffer>,
std::unique_ptr<llvm::MemoryBuffer>>
getInputBufferAndModuleDocBufferIfPresent(const InputFile &input);
ModuleBuffers getInputBuffersIfPresent(const InputFile &input);

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 5, 2019

Member

While you're here, may be worth making this Optional, rather than relying on a null ModuleBuffer as a sentinel.

return {
std::move(*inputFileOrErr),
moduleDocBuffer.hasValue() ? std::move(*moduleDocBuffer): nullptr,
moduleSourceInfoBuffer.hasValue() ? std::move(*moduleSourceInfoBuffer): nullptr

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 5, 2019

Member

Nitpick: missing space before ternary operator colon.

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 5, 2019

Member

You can also simplify this with std::move(moduleDocBuffer).getValueOr(nullptr). (I think this is the correct nesting.)

std::string NonPrivatePath = moduleSourceInfoFilePath.str().str();
StringRef fileName = llvm::sys::path::filename(NonPrivatePath);
llvm::sys::path::remove_filename(moduleSourceInfoFilePath);
llvm::sys::path::append(moduleSourceInfoFilePath, "Private");

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 5, 2019

Member

Per discussion, still suggest changing this to "Project", to save "Private" for something closer in meaning to "private headers".

@@ -1222,13 +1223,272 @@ bool ModuleFile::readModuleDocIfPresent() {
return true;
}

class ModuleFile::BasicDeclLocTableInfo {

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 5, 2019

Member

Nitpick: Can you put all this in another file? I know the doc stuff is mixed up here but it probably shouldn't be.

/// to make backwards-compatible changes using the LLVM bitcode format.
///
/// \sa DECL_LOCS_BLOCK_ID
namespace sourceinfo_block {

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 5, 2019

Member

Is it the DECL_LOCS block or the SOURCEINFO block?

namespace sourceinfo_block {
enum RecordKind {
BASIC_DECL_LOCS = 1,
DECL_USRS = 96,

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 5, 2019

Member

Why the jump to 96?

@@ -502,7 +502,390 @@ void serialization::writeDocToStream(raw_ostream &os, ModuleOrSourceFile DC,

S.writeToStream(os);
}
namespace {
struct LineColoumn {

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 5, 2019

Member

Typo: Coloumn

// EndLoc
dataLength += LineColumnLen;
endian::Writer writer(out, little);
writer.write<uint32_t>(keyLength);

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 5, 2019

Member

If the length is constant you actually don't have to serialize it at all; returning it is sufficient.

WRITE_LINE_COLUMN(Loc)
WRITE_LINE_COLUMN(NameLoc);
WRITE_LINE_COLUMN(StartLoc);
WRITE_LINE_COLUMN(EndLoc);

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 5, 2019

Member

Inconsistent about the semicolons here. (I liked the lambda better, honestly.)

Copy link
Contributor

left a comment

I just looked over it quickly, but nothing jumped out at me to comment upon. (I'll miss @jrose-apple 's reviews!) Let me know if you want another pair of eyes to go over it in depth.

@nkcsgexi

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2019

@jrose-apple The most recent commit refactored the data structure as we discussed to use only one hash-table. Could you take another look?

@nkcsgexi nkcsgexi force-pushed the nkcsgexi:deserialize-source-info branch from 073e397 to 5257bee Oct 7, 2019
@nkcsgexi

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

@swift-ci please smoke test

Copy link
Contributor

left a comment

I'm not going to pretend I had the attention span to read every line in detail, but it looked alright in my skim. If there's something specific you want me to examine more minutely, please let me know.

std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer,
std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer) = 0;
std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer,
std::unique_ptr<llvm::MemoryBuffer> *ModuleSourceInfoBuffer) = 0;

This comment has been minimized.

Copy link
@brentdax

brentdax Oct 8, 2019

Contributor

These now-three files are passed together so often that I'm beginning to think we should have an abstraction to represent this grouping. ModuleFiles<StringRef> ModuleFilenames, ModuleFiles<std::unique_ptr<llvm::MemoryBuffer> *> ModuleBuffers.

include/swift/AST/RawComment.h Show resolved Hide resolved
@nkcsgexi nkcsgexi force-pushed the nkcsgexi:deserialize-source-info branch from 6ab1c06 to 9ea9192 Oct 8, 2019
@nkcsgexi

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

@swift-ci please smoke test

Copy link
Member

left a comment

More comments on everything except SerializeDoc.cpp.

@@ -2793,7 +2793,7 @@ static void chooseModuleAuxiliaryOutputFilePath(Compilation &C,
StringRef workingDirectory,
CommandOutput *Output,
file_types::ID fileID,
bool isPrivate,
bool isProject = false,

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 9, 2019

Member

Nitpick: "isProject" doesn't feel as descriptive if you don't already know what this is doing. Maybe "isProjectOnlyContent" or "shouldUseProjectFolder"?

lib/Frontend/Frontend.cpp Show resolved Hide resolved
llvm::SmallString<128> moduleSourceInfoFilePath(input.file());
llvm::sys::path::replace_extension(moduleSourceInfoFilePath,
file_types::getExtension(file_types::TY_SwiftSourceInfoFile));
std::string NonPrivatePath = moduleSourceInfoFilePath.str().str();

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 9, 2019

Member

Nitpicks: this should be a SmallString too, and it should have a lowerCamelCase name, and I don't love that the name is described in terms of the other path.

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 9, 2019

Member

Actually, I don't think we need to do the "Project" thing for the partial modules.

@@ -1222,13 +1223,164 @@ bool ModuleFile::readModuleDocIfPresent() {
return true;
}

class ModuleFile::DeclUSRTableInfo {
public:

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 9, 2019

Member

Nitpick: indentation

return llvm::djbHash(key, SWIFTSOURCEINFO_HASH_SEED);
}

static bool EqualKey(internal_key_type lhs, internal_key_type rhs) { return lhs == rhs; }

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 9, 2019

Member

Nitpick: 80 cols

@@ -263,10 +263,43 @@ std::error_code SerializedModuleLoaderBase::openModuleDocFile(
return std::error_code();
}

std::error_code
SerializedModuleLoaderBase::openModuleSourceInfoFile(AccessPathElem ModuleID,

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 9, 2019

Member

I mean, here is fine, just generalize it.

using llvm::BCVBR;

/// Magic number for serialized source info files.
const unsigned char SWIFTSOURCEINFO_SIGNATURE[] = { 0xD6, 0x9C, 0xB7, 0x23 };

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 9, 2019

Member

Where does this number come from? :-)

This comment has been minimized.

Copy link
@nkcsgexi

nkcsgexi Oct 9, 2019

Author Contributor

I made these number up. Is there a recommended way?

This comment has been minimized.

Copy link
@harlanhaskins

harlanhaskins Oct 9, 2019

Collaborator

Concept: use the UTF8 bytes of 📒

This comment has been minimized.

Copy link
@nkcsgexi

nkcsgexi Oct 10, 2019

Author Contributor

I end up using 🏎️as the magic number :)

///
/// Increment this value when making a backwards-incompatible change, i.e. where
/// an \e old compiler will \e not be able to read the new format. This should
/// be rare. When incrementing this value, reset SWIFTDOC_VERSION_MINOR to 0.

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 9, 2019

Member

Don't forget to update the comment!


using BasicDeclLocsLayout = BCRecordLayout<
BASIC_DECL_LOCS, // record ID
BCBlob // an array of fixed size location data

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 9, 2019

Member

Not very extensible, but probably good enough.

lib/Serialization/ModuleFile.cpp Show resolved Hide resolved
nkcsgexi added 12 commits Oct 4, 2019
…are importing a module from a Swift interface file
… in .swiftsourceinfo file

For .swiftdoc file, we don't expose doc-comments for underscored symbols. But this
seems to be an unnecessary constraint on .swiftsourceinfo file since we put
these symbols in .swiftinterface files anyway.
After this change, we only use one single hash table for USR to USR id
mapping. The basic source locations are an array of fixed length
records that could be retrieved by using the USR id since each
USR id is guaranteed to be associated with one basic location entry.

The source file paths are refactored to a blob of 0-terminated strings.
Decl locations use offset in this blob to refer to the source file path
where the decl was defined.
Having both Loc and NameLoc fields seems to be redundant.
…/Project

This directory should be excluded during installation since the content is only
used for local development. swiftsourceinfo file is currently emitted to this directory.
@nkcsgexi nkcsgexi force-pushed the nkcsgexi:deserialize-source-info branch from 2346edc to 014f863 Oct 9, 2019
@nkcsgexi

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

@swift-ci please smoke test

nkcsgexi added 2 commits Oct 9, 2019
After having this function, the serialization logic doesn't have to
know where the source locations are coming from (.swift or .swiftmodule).
@nkcsgexi

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2019

@swift-ci please smoke test

@nkcsgexi

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2019

@swift-ci Please smoke test compiler performance

@nkcsgexi

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2019

@swift-ci please test

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

Build failed
Swift Test OS X Platform
Git Sha - effa580

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

Build failed
Swift Test Linux Platform
Git Sha - effa580

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

Summary for master smoketest

Unexpected test results, excluded stats for ReactiveCocoa

Regressions found (see below)

Debug

debug brief

Regressed (1)
name old new delta delta_pct
time.swift-driver.wall 9.2s 9.4s 165.8ms 1.8% ⛔️
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (2)
name old new delta delta_pct
Frontend.NumInstructionsExecuted 109,837,790,830 109,978,620,952 140,830,122 0.13%
LLVM.NumLLVMBytesOutput 6,137,688 6,137,632 -56 -0.0%

debug detailed

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (19)
name old new delta delta_pct
AST.NumLoadedModules 660 660 0 0.0%
AST.NumTotalClangImportedEntities 3,642 3,642 0 0.0%
IRModule.NumIRBasicBlocks 17,897 17,897 0 0.0%
IRModule.NumIRFunctions 10,605 10,605 0 0.0%
IRModule.NumIRGlobals 8,800 8,800 0 0.0%
IRModule.NumIRInsts 308,495 308,495 0 0.0%
IRModule.NumIRValueSymbols 18,429 18,429 0 0.0%
LLVM.NumLLVMBytesOutput 6,137,688 6,137,632 -56 -0.0%
SILModule.NumSILGenFunctions 5,372 5,372 0 0.0%
SILModule.NumSILOptFunctions 7,270 7,270 0 0.0%
Sema.NumConformancesDeserialized 11,698 11,698 0 0.0%
Sema.NumConstraintScopes 38,796 38,796 0 0.0%
Sema.NumDeclsDeserialized 113,328 113,328 0 0.0%
Sema.NumDeclsValidated 4,728 4,728 0 0.0%
Sema.NumFunctionsTypechecked 2,034 2,034 0 0.0%
Sema.NumGenericSignatureBuilders 4,054 4,054 0 0.0%
Sema.NumLazyIterableDeclContexts 18,445 18,445 0 0.0%
Sema.NumTypesDeserialized 43,909 43,909 0 0.0%
Sema.NumTypesValidated 7,546 7,546 0 0.0%

Release

release brief

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (3)
name old new delta delta_pct
Frontend.NumInstructionsExecuted 145,054,899,351 145,028,048,236 -26,851,115 -0.02%
LLVM.NumLLVMBytesOutput 6,882,412 6,882,480 68 0.0%
time.swift-driver.wall 17.0s 17.1s 66.6ms 0.39%

release detailed

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (19)
name old new delta delta_pct
AST.NumLoadedModules 76 76 0 0.0%
AST.NumTotalClangImportedEntities 2,080 2,080 0 0.0%
IRModule.NumIRBasicBlocks 18,159 18,159 0 0.0%
IRModule.NumIRFunctions 9,984 9,984 0 0.0%
IRModule.NumIRGlobals 8,917 8,917 0 0.0%
IRModule.NumIRInsts 200,483 200,483 0 0.0%
IRModule.NumIRValueSymbols 18,077 18,077 0 0.0%
LLVM.NumLLVMBytesOutput 6,882,412 6,882,480 68 0.0%
SILModule.NumSILGenFunctions 4,178 4,178 0 0.0%
SILModule.NumSILOptFunctions 6,218 6,218 0 0.0%
Sema.NumConformancesDeserialized 12,395 12,395 0 0.0%
Sema.NumConstraintScopes 38,546 38,546 0 0.0%
Sema.NumDeclsDeserialized 31,875 31,875 0 0.0%
Sema.NumDeclsValidated 3,198 3,198 0 0.0%
Sema.NumFunctionsTypechecked 2,034 2,034 0 0.0%
Sema.NumGenericSignatureBuilders 1,551 1,551 0 0.0%
Sema.NumLazyIterableDeclContexts 4,261 4,261 0 0.0%
Sema.NumTypesDeserialized 16,544 16,544 0 0.0%
Sema.NumTypesValidated 4,808 4,808 0 0.0%
@nkcsgexi

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2019

@swift-ci please test

@nkcsgexi

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2019

Talked with @jrose-apple about this, we plan to merge it for now to unblock progress. Future code review comments, if any, will be addressed by separate PRs.

@nkcsgexi nkcsgexi merged commit c9f1900 into apple:master Oct 10, 2019
4 checks passed
4 checks passed
Swift Test Linux Platform No test results found.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform No test results found.
Details
Swift Test OS X Platform (smoke test)
Details
@compnerd

This comment has been minimized.

compnerd added a commit to compnerd/apple-swift that referenced this pull request Oct 11, 2019
try to invoke a std::unique_ptr<>'s copy constructor, which is deleted.
Either move the unique_ptr or pass along a nullptr.  This should repair
the Windows build.
@nkcsgexi nkcsgexi deleted the nkcsgexi:deserialize-source-info branch Oct 11, 2019
Copy link
Member

left a comment

Further comments, none of which are too serious.

openModuleSourceInfoFileIfPresent(AccessPathElem ModuleID,
StringRef ModulePath,
StringRef ModuleSourceInfoFileName,
std::unique_ptr<llvm::MemoryBuffer> *ModuleSourceInfoBuffer);

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 11, 2019

Member

Nitpick: Swift/LLVM style doesn't right-align parameters that are too long. Instead, give up on aligning to the open paren and double-indent from the left. (This is what clang-format will do, I believe.)

}
}
return true;
}

bool ide::printDeclUSR(const Decl *D, raw_ostream &OS) {
if (D->isImplicit())
return true;

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 11, 2019

Member

This test doesn't really belong here; can you put it at the call site instead? We should be able to print USRs for implicit things in general.

failed = true;
return None;
}

// FIXME: The fact that this test happens twice, for some cases,
// suggests that setupInputs could use another round of refactoring.
if (serialization::isSerializedAST(buffers.first->getBuffer())) {
if (serialization::isSerializedAST(buffers->ModuleBuffer->getBuffer())) {
PartialModules.push_back(

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 11, 2019

Member

Is this just PartialModules.push_back(std::move(buffers))?

AccessPathElem ModuleID,
StringRef ModulePath,
StringRef ModuleSourceInfoFilename,
std::unique_ptr<llvm::MemoryBuffer> *ModuleSourceInfoBuffer) {

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 11, 2019

Member

Same comment about right-alignment here.

// SECOND: Inputs/def_comments.swift:28:13: Var/decl_var_2 RawComment=none BriefComment=none DocCommentAsXML=none
// SECOND: Inputs/def_comments.swift:28:25: Var/decl_var_3 RawComment=none BriefComment=none DocCommentAsXML=none
// SECOND: Inputs/def_comments.swift:28:25: Var/decl_var_3 RawComment=none BriefComment=none DocCommentAsXML=none
// SECOND: NonExistingSource.swift:100000:13: Func/functionAfterPoundSourceLoc

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 11, 2019

Member

Should this include a test that it's made absolute? Even if that's not important behavior, we probably still want to know if it changes.

<< ":" << LineAndColumn.first << ":" << LineAndColumn.second << ": ";
} else {
printSerializedLoc(D);

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 11, 2019

Member

You should be able to always go through this path now.

Buffer.append(Text);
Buffer.push_back('\0');
}
return IndexMap[Text];

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 11, 2019

Member

Performance nitpick: this will do 2-3 hash lookups of the text, and while that's not super important, you can do better with something like this:

auto iterAndIsNew = IndexMap.insert({Text, Buffer.size()});
if (iterAndIsNew.second) {
  Buffer.append(Text);
  Buffer.push_back('\0');
}
return (*IterAndIsNew.first).getValue();
std::unique_ptr<ModuleFile::SerializedDeclUSRTable>
ModuleFile::readDeclUSRsTable(ArrayRef<uint64_t> fields, StringRef blobData) {
if (fields.empty() || blobData.empty())
return nullptr;

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Oct 11, 2019

Member

Funky indentation here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.