[PATCH] Replace OwningPtr with std::unique_ptr.

Eric Christopher echristo at gmail.com
Wed Mar 12 10:29:32 PDT 2014


(adding Rui to the mail thread since he was mentioned...)

On Wed, Mar 12, 2014 at 10:27 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>   Mostly looks good, though lld's a bit out of my depth so I'll leave it to someone else (an owner or someone otherwise feeling sufficient authority in the area) to sign off, perhaps Rui.
>
>
> ================
> Comment at: lib/Passes/RoundTripYAMLPass.cpp:48
> @@ -46,4 +47,3 @@
>
> -  if (buff->getBufferSize() < MAX_YAML_FILE_SIZE) {
> -    std::unique_ptr<MemoryBuffer> mb(buff.take());
> +  if (mb->getBufferSize() < MAX_YAML_FILE_SIZE) {
>      error_code ec = _context.registry().parseFile(mb, _yamlFile);
> ----------------
> Any particular reason to change the name of the variable? The old one seemed about as good & less churn.
>
> ================
> Comment at: lib/Core/InputGraph.cpp:143
> @@ -141,3 +142,3 @@
>
> -  if (error_code ec = MemoryBuffer::getFileOrSTDIN(filePath, opmb))
> +  if (error_code ec = MemoryBuffer::getFileOrSTDIN(filePath, mb))
>      return ec;
> ----------------
> Could be worth checking, but I assume "MemoryBuffer::getFileOrSTDIN" leaves the pointer null if it fails - so you could just avoid the temporary 'mb' and pass '_buffer' here, maybe.. (but yeah, perfectly fine to leave this as-is - it's a strict improvement in any case)
>
> ================
> Comment at: lib/ReaderWriter/PECOFF/ReaderCOFF.cpp:282
> @@ -281,3 +281,3 @@
>    }
> -  bin.take();
> +  bin.release();
>
> ----------------
> that's an interesting bit of memory (non)ownership... any idea what's going on here?
>
> Oh, I see - this is an issue with unique_ptr going through dyn_cast. Ew.
>
> It'd be great to improve dyn_cast to handle this sort of thing.
>
>
> http://llvm-reviews.chandlerc.com/D3055
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits




More information about the llvm-commits mailing list