[PATCH] D27646: Refactor BitcodeReader: move Metadata and ValueId handling in their own class/file

[Duncan P. N. Exon Smith via llvm-commits]

> On 2016-Dec-09, at 17:56, Mehdi AMINI via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> mehdi_amini updated this revision to Diff 80986.
> mehdi_amini added a comment.
> 
> Remove IsMetadataMaterialized, didn't find a use for it?
> 
> 
> https://reviews.llvm.org/D27646
> 
> Files:
>  llvm/lib/Bitcode/Reader/BitcodeReader.cpp
>  llvm/lib/Bitcode/Reader/CMakeLists.txt
>  llvm/lib/Bitcode/Reader/MetadataLoader.cpp
>  llvm/lib/Bitcode/Reader/MetadataLoader.h
>  llvm/lib/Bitcode/Reader/ValueList.cpp
>  llvm/lib/Bitcode/Reader/ValueList.h
> 

> Index: llvm/lib/Bitcode/Reader/MetadataLoader.h
> ===================================================================
> --- /dev/null
> +++ llvm/lib/Bitcode/Reader/MetadataLoader.h
> @@ -0,0 +1,69 @@
> +//===-- Bitcode/Reader/MetadataLoader.h - Number values --------*- C++ -*-====//

"Number values" seems inaccurate.  "Load metadata"

> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +//
> +// This class handles loading Metadatas.
> +//
> +//===----------------------------------------------------------------------===//
> +
> +#ifndef LLVM_LIB_BITCODE_READER_METADATALOADER_H
> +#define LLVM_LIB_BITCODE_READER_METADATALOADER_H
> Index: llvm/lib/Bitcode/Reader/BitcodeReader.cpp
> ===================================================================
> --- llvm/lib/Bitcode/Reader/BitcodeReader.cpp
> +++ llvm/lib/Bitcode/Reader/BitcodeReader.cpp
> @@ -526,7 +395,7 @@
>  
>    std::vector<Type*> TypeList;
>    BitcodeReaderValueList ValueList;
> -  BitcodeReaderMetadataList MetadataList;
> +  std::unique_ptr<MetadataLoader> MetadataList;

Could/should this be an Optional<> instead?

Is the name still appropriate?  I might suggest MDs or MDLoader instead of
MetadataList.  Up to you though.

>    std::vector<Comdat *> ComdatList;
>    SmallVector<Instruction *, 64> InstructionList;
>  
> @@ -3959,17 +2661,17 @@
>            return Err;
>          break;
>        case bitc::METADATA_BLOCK_ID:
> -        if (ShouldLazyLoadMetadata && !IsMetadataMaterialized) {
> +        if (ShouldLazyLoadMetadata) {

Why did you remove this use of IsMetadataMaterialized?  I'd rather that be
committed separately (if it's the right thing to do).

>            if (Error Err = rememberAndSkipMetadata())
>              return Err;
>            break;
>          }
>          assert(DeferredMetadataInfo.empty() && "Unexpected deferred metadata");
> -        if (Error Err = parseMetadata(true))
> +        if (Error Err = MetadataList->parseModuleMetadata())
>            return Err;
>          break;
>        case bitc::METADATA_KIND_BLOCK_ID:
> -        if (Error Err = parseMetadataKinds())
> +        if (Error Err = MetadataList->parseMetadataKinds())
>            return Err;
>          break;
>        case bitc::FUNCTION_BLOCK_ID: