Skip to content

[Serialization] Remove several unnecessary bits of helper logic #26735

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Aug 20, 2019
Merged

[Serialization] Remove several unnecessary bits of helper logic #26735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
12fc353
[Serialization] Remove superfluous helper function
jrose-apple Aug 19, 2019
c6f033d
[Serialization] Use existing logic to print a hierarchical module name
jrose-apple Aug 19, 2019
31d5802
[Serialization] Remove a hack around serializing 'final'
jrose-apple Aug 20, 2019
f12373b
[Serialization] Minor cleanups, no functionality change
jrose-apple Aug 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/swift/AST/Module.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ class ModuleDecl : public DeclContext, public TypeDecl {

/// This is a convenience function that writes the entire name, in forward
/// order, to \p out.
void printForward(raw_ostream &out) const;
void printForward(raw_ostream &out, StringRef delim = ".") const;
};

private:
Expand Down
5 changes: 3 additions & 2 deletions lib/AST/Module.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1200,11 +1200,12 @@ ModuleDecl::ReverseFullNameIterator::operator++() {
}

void
ModuleDecl::ReverseFullNameIterator::printForward(raw_ostream &out) const {
ModuleDecl::ReverseFullNameIterator::printForward(raw_ostream &out,
StringRef delim) const {
SmallVector<StringRef, 8> elements(*this, {});
swift::interleave(swift::reversed(elements),
[&out](StringRef next) { out << next; },
[&out] { out << '.'; });
[&out, delim] { out << delim; });
}

void
Expand Down
86 changes: 19 additions & 67 deletions lib/Serialization/Serialization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,36 +459,6 @@ static bool shouldSerializeAsLocalContext(const DeclContext *DC) {
!isa<SubscriptDecl>(DC);
}

static const Decl *getDeclForContext(const DeclContext *DC) {
switch (DC->getContextKind()) {
case DeclContextKind::Module:
// Use a null decl to represent the module.
return nullptr;
case DeclContextKind::FileUnit:
return getDeclForContext(DC->getParent());
case DeclContextKind::SerializedLocal:
llvm_unreachable("Serialized local contexts should only come from deserialization");
case DeclContextKind::Initializer:
case DeclContextKind::AbstractClosureExpr:
// FIXME: What about default functions?
llvm_unreachable("shouldn't serialize decls from anonymous closures");
case DeclContextKind::GenericTypeDecl:
return cast<GenericTypeDecl>(DC);
case DeclContextKind::ExtensionDecl:
return cast<ExtensionDecl>(DC);
case DeclContextKind::TopLevelCodeDecl:
llvm_unreachable("shouldn't serialize the main module");
case DeclContextKind::AbstractFunctionDecl:
return cast<AbstractFunctionDecl>(DC);
case DeclContextKind::SubscriptDecl:
return cast<SubscriptDecl>(DC);
case DeclContextKind::EnumElementDecl:
return cast<EnumElementDecl>(DC);
}

llvm_unreachable("Unhandled DeclContextKind in switch.");
}

namespace {
struct Accessors {
uint8_t OpaqueReadOwnership;
Expand Down Expand Up @@ -634,7 +604,7 @@ DeclContextID Serializer::addDeclContextRef(const DeclContext *DC) {
if (shouldSerializeAsLocalContext(DC))
addLocalDeclContextRef(DC);
else
addDeclRef(getDeclForContext(DC));
addDeclRef(DC->getAsDecl());

auto &id = DeclContextIDs[DC];
if (id)
Expand Down Expand Up @@ -1023,23 +993,19 @@ void Serializer::writeHeader(const SerializationOptions &options) {
}
}

using ImportPathBlob = llvm::SmallString<64>;
static void flattenImportPath(const ModuleDecl::ImportedModule &import,
ImportPathBlob &out) {
SmallVector<StringRef, 4> reverseSubmoduleNames(
import.second->getReverseFullModuleName(), {});

interleave(reverseSubmoduleNames.rbegin(), reverseSubmoduleNames.rend(),
[&out](StringRef next) { out.append(next); },
[&out] { out.push_back('\0'); });
SmallVectorImpl<char> &out) {
llvm::raw_svector_ostream outStream(out);
import.second->getReverseFullModuleName().printForward(outStream,
StringRef("\0", 1));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised you can't just say "\0" here. Would it try to use strlen() if you did?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's the default behavior for StringRef. The subclass StringLiteral knows how to handle embedded NULs but that's more verbose than just doing this.


if (import.first.empty())
return;

out.push_back('\0');
outStream << '\0';
assert(import.first.size() == 1 && "can only handle top-level decl imports");
auto accessPathElem = import.first.front();
out.append(accessPathElem.first.str());
outStream << accessPathElem.first.str();
}

uint64_t getRawModTimeOrHash(const SerializationOptions::FileDependency &dep) {
Expand Down Expand Up @@ -1152,7 +1118,7 @@ void Serializer::writeInputBlock(const SerializationOptions &options) {
continue;
}

ImportPathBlob importPath;
SmallString<64> importPath;
flattenImportPath(import, importPath);

serialization::ImportControl stableImportControl;
Expand Down Expand Up @@ -1538,7 +1504,7 @@ Serializer::writeConformance(ProtocolConformanceRef conformanceRef,
switch (conformance->getKind()) {
case ProtocolConformanceKind::Normal: {
auto normal = cast<NormalProtocolConformance>(conformance);
if (!isDeclXRef(getDeclForContext(normal->getDeclContext()))
if (!isDeclXRef(normal->getDeclContext()->getAsDecl())
&& !isa<ClangModuleUnit>(normal->getDeclContext()
->getModuleScopeContext())) {
// A normal conformance in this module file.
Expand Down Expand Up @@ -2006,12 +1972,6 @@ static void verifyAttrSerializable(const KIND ## Decl *D) {\
static void verifyAttrSerializable(const Decl *D) {}
#endif

static inline unsigned getOptionalOrZero(const llvm::Optional<unsigned> &X) {
if (X.hasValue())
return X.getValue();
return 0;
}

bool Serializer::isDeclXRef(const Decl *D) const {
const DeclContext *topLevel = D->getDeclContext()->getModuleScopeContext();
if (topLevel->getParentModule() != M)
Expand Down Expand Up @@ -2046,7 +2006,7 @@ void Serializer::writeDeclContext(const DeclContext *DC) {
case DeclContextKind::GenericTypeDecl:
case DeclContextKind::ExtensionDecl:
case DeclContextKind::EnumElementDecl:
declOrDeclContextID = addDeclRef(getDeclForContext(DC));
declOrDeclContextID = addDeclRef(DC->getAsDecl());
isDecl = true;
break;

Expand Down Expand Up @@ -2416,8 +2376,8 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
if (X##_Val.hasValue()) {\
const auto &Y = X##_Val.getValue();\
X##_Major = Y.getMajor();\
X##_Minor = getOptionalOrZero(Y.getMinor());\
X##_Subminor = getOptionalOrZero(Y.getSubminor());\
X##_Minor = Y.getMinor().getValueOr(0);\
X##_Subminor = Y.getSubminor().getValueOr(0);\
X##_HasMinor = Y.getMinor().hasValue();\
X##_HasSubminor = Y.getSubminor().hasValue();\
}
Expand Down Expand Up @@ -2848,13 +2808,6 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
for (auto Attr : D->getAttrs())
writeDeclAttribute(Attr);

if (auto VD = dyn_cast<ValueDecl>(D)) {
// Hack: synthesize a 'final' attribute if finality was inferred.
if (VD->isFinal() && !D->getAttrs().hasAttribute<FinalAttr>())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this become impossible?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worthwhile to have an assertion here to double-check that FinalAttr is in sync or is that not useful because correctness is guaranteed by construction?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'll add it to the AST verifier; that's where such correct-by-construction assertions belong.

writeDeclAttribute(
new (D->getASTContext()) FinalAttr(/*Implicit=*/false));
}

if (auto *value = dyn_cast<ValueDecl>(D))
writeDiscriminatorsIfNeeded(value);

Expand Down Expand Up @@ -2935,8 +2888,7 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {

// Reverse the list, and write the parameter lists, from outermost
// to innermost.
std::reverse(allGenericParams.begin(), allGenericParams.end());
for (auto *genericParams : allGenericParams)
for (auto *genericParams : swift::reversed(allGenericParams))
writeGenericParams(genericParams);

writeMembers(id, extension->getMembers(), isClassExtension);
Expand Down Expand Up @@ -5035,7 +4987,7 @@ void swift::serializeToBuffers(
std::unique_ptr<llvm::MemoryBuffer> *moduleDocBuffer,
const SILModule *M) {

assert(options.OutputPath && options.OutputPath[0] != '\0');
assert(!StringRef::withNullAsEmpty(options.OutputPath).empty());
{
SharedTimer timer("Serialization, swiftmodule, to buffer");
llvm::SmallString<1024> buf;
Expand All @@ -5054,7 +5006,7 @@ void swift::serializeToBuffers(
std::move(buf), options.OutputPath);
}

if (options.DocOutputPath && options.DocOutputPath[0] != '\0') {
if (!StringRef::withNullAsEmpty(options.DocOutputPath).empty()) {
SharedTimer timer("Serialization, swiftdoc, to buffer");
llvm::SmallString<1024> buf;
llvm::raw_svector_ostream stream(buf);
Expand All @@ -5074,12 +5026,12 @@ void swift::serializeToBuffers(
void swift::serialize(ModuleOrSourceFile DC,
const SerializationOptions &options,
const SILModule *M) {
assert(options.OutputPath && options.OutputPath[0] != '\0');
assert(!StringRef::withNullAsEmpty(options.OutputPath).empty());

if (strcmp("-", options.OutputPath) == 0) {
if (StringRef(options.OutputPath) == "-") {
// Special-case writing to stdout.
Serializer::writeToStream(llvm::outs(), DC, M, options);
assert(!options.DocOutputPath || options.DocOutputPath[0] == '\0');
assert(StringRef::withNullAsEmpty(options.DocOutputPath).empty());
return;
}

Expand All @@ -5093,7 +5045,7 @@ void swift::serialize(ModuleOrSourceFile DC,
if (hadError)
return;

if (options.DocOutputPath && options.DocOutputPath[0] != '\0') {
if (!StringRef::withNullAsEmpty(options.DocOutputPath).empty()) {
(void)withOutputFile(getContext(DC).Diags,
options.DocOutputPath,
[&](raw_ostream &out) {
Expand Down