[flang] Changes based on review comments

Original-commit: flang-compiler/f18@fc4c0c39d5
Reviewed-on: https://github.com/flang-compiler/f18/pull/160

flang/lib/semantics/mod-file.cc

@@ -71,32 +71,36 @@ void ModFileWriter::WriteChildren(const Scope &scope) {
 }
 
 void ModFileWriter::WriteOne(const Scope &scope) {
-  auto *symbol{scope.symbol()};
-  if (scope.kind() != Scope::Kind::Module) {
+  if (scope.kind() == Scope::Kind::Module) {
+    auto *symbol{scope.symbol()};
+    if (!symbol->test(Symbol::Flag::ModFile)) {
+      Write(*symbol);
+    }
+    WriteChildren(scope);  // write out submodules
+  }
+}
+
+// Write the module file for symbol, which must be a module or submodule.
+void ModFileWriter::Write(const Symbol &symbol) {
+  auto *ancestor{symbol.get<ModuleDetails>().ancestor()};
+  auto ancestorName{ancestor ? ancestor->name().ToString() : ""s};
+  auto path{ModFilePath(dir_, symbol.name(), ancestorName)};
+  unlink(path.data());
+  std::ofstream os{path};
+  PutSymbols(*symbol.scope());
+  std::string all{GetAsString(symbol)};
+  auto header{GetHeader(all)};
+  os << header << all;
+  os.close();
+  if (!os) {
+    errors_.emplace_back(
+        "Error writing %s: %s"_err_en_US, path.c_str(), std::strerror(errno));
     return;
   }
-  if (!symbol->test(Symbol::Flag::ModFile)) {
-    auto *ancestor{symbol->get<ModuleDetails>().ancestor()};
-    auto ancestorName{ancestor ? ancestor->name().ToString() : ""s};
-    auto path{ModFilePath(dir_, symbol->name(), ancestorName)};
-    unlink(path.data());
-    std::ofstream os{path};
-    PutSymbols(scope);
-    std::string all{GetAsString(*symbol)};
-    auto header{GetHeader(all)};
-    os << header << all;
-    os.close();
-    if (!os) {
-      errors_.emplace_back(
-          "Error writing %s: %s"_err_en_US, path.c_str(), std::strerror(errno));
-      return;
-    }
-    if (!MakeReadonly(path)) {
-      errors_.emplace_back("Error changing permissions on %s: %s"_en_US,
-          path.c_str(), std::strerror(errno));
-    }
+  if (!MakeReadonly(path)) {
+    errors_.emplace_back("Error changing permissions on %s: %s"_en_US,
+        path.c_str(), std::strerror(errno));
   }
-  WriteChildren(scope);  // write out submodules
 }
 
 // Return the entire body of the module file
@@ -406,14 +410,13 @@ Scope *ModFileReader::Read(const SourceName &name, Scope *ancestor) {
             name.ToString(), *path));
     return nullptr;
   }
-  Scope *parentScope;
+  Scope *parentScope;  // the scope this module/submodule goes into
   if (!ancestor) {
-    // module: goes into global scope
     parentScope = &Scope::globalScope;
+  } else if (auto *parent{GetSubmoduleParent(*parseTree)}) {
+    parentScope = Read(*parent, ancestor);
   } else {
-    // submodule: goes into parent module/submodule
-    auto *parent{GetSubmoduleParent(*parseTree)};
-    parentScope = parent ? Read(*parent, ancestor) : ancestor;
+    parentScope = ancestor;
   }
   ResolveNames(*parentScope, *parseTree, parsing.cooked(), directories_);
   const auto &it{parentScope->find(name)};

flang/lib/semantics/mod-file.h

@@ -66,6 +66,7 @@ private:
 
   void WriteChildren(const Scope &);
   void WriteOne(const Scope &);
+  void Write(const Symbol &);
   std::string GetAsString(const Symbol &);
   std::string GetHeader(const std::string &);
   void PutSymbols(const Scope &);

flang/lib/semantics/resolve-names.cc

@@ -415,7 +415,7 @@ private:
       const SourceName &useName);
   Symbol &BeginModule(const SourceName &, bool isSubmodule,
       const std::optional<parser::ModuleSubprogramPart> &);
-  Scope *FindModule(const SourceName &, Scope * = nullptr);
+  Scope *FindModule(const SourceName &, Scope *ancestor = nullptr);
 };
 
 class InterfaceVisitor : public virtual ScopeHandler {
@@ -1333,7 +1333,7 @@ bool ModuleVisitor::Pre(const parser::Submodule &x) {
   }
   PushScope(*parentScope);  // submodule is hosted in parent
   auto &symbol{BeginModule(name, true, subpPart)};
-  if (ancestor->AddSubmodule(name, &CurrScope())) {
+  if (!ancestor->AddSubmodule(name, &CurrScope())) {
     Say(name, "Module '%s' already has a submodule named '%s'"_err_en_US,
         ancestorName, name);
   }

flang/lib/semantics/scope.cc

@@ -56,9 +56,8 @@ Scope *Scope::FindSubmodule(const SourceName &name) const {
     return it->second;
   }
 }
-Scope *Scope::AddSubmodule(const SourceName &name, Scope *submodule) {
-  auto pair{submodules_.emplace(name, submodule)};
-  return !pair.second ? pair.first->second : nullptr;
+bool Scope::AddSubmodule(const SourceName &name, Scope *submodule) {
+  return submodules_.emplace(name, submodule).second;
 }
 DerivedTypeSpec &Scope::MakeDerivedTypeSpec(const SourceName &name) {
   derivedTypeSpecs_.emplace_back(name);

flang/lib/semantics/scope.h

@@ -104,9 +104,9 @@ public:
   const std::list<Scope> &children() const { return children_; }
 
   // For Module scope, maintain a mapping of all submodule scopes with this
-  // module as its ancestor module.
+  // module as its ancestor module. AddSubmodule returns false if already there.
   Scope *FindSubmodule(const SourceName &) const;
-  Scope *AddSubmodule(const SourceName &, Scope *);
+  bool AddSubmodule(const SourceName &, Scope *);
 
   DerivedTypeSpec &MakeDerivedTypeSpec(const SourceName &);