This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Parser] Make parse{Attribute,Type} null-terminate input
ClosedPublic

Authored by rkayaith on Mar 2 2023, 1:18 PM.

Details

Summary

parseAttribute and parseType require null-terminated strings as
input, but this isn't great considering the argument type is
StringRef. This changes them to copy to a null-terminated buffer by
default, with a isKnownNullTerminated flag added to disable the
copying.

closes #58964

Diff Detail

Event Timeline

rkayaith created this revision.Mar 2 2023, 1:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2023, 1:18 PM
rkayaith requested review of this revision.Mar 2 2023, 1:18 PM
kuhar added a subscriber: kuhar.Mar 2 2023, 1:23 PM
kuhar added inline comments.
mlir/include/mlir/AsmParser/AsmParser.h
71

nit: typeStr.str()? Also everywhere else

rkayaith updated this revision to Diff 501961.Mar 2 2023, 1:26 PM

std::string() -> .str()

kuhar added inline comments.Mar 2 2023, 1:28 PM
mlir/unittests/Parser/ParserTest.cpp
99

nit: StringRef attrAsm("999", 1);

rkayaith updated this revision to Diff 501989.Mar 2 2023, 2:28 PM
rkayaith marked 2 inline comments as done.

address review comment

kuhar accepted this revision.Mar 2 2023, 2:49 PM

LGTM but please wait for a second review before submitting

This revision is now accepted and ready to land.Mar 2 2023, 2:49 PM
lattner added a subscriber: lattner.Mar 2 2023, 9:17 PM

Cool. Suggestion: instead of duplicating the APIs, you could add a , bool isKnownNullTerminated = false flag. That keeps the API simpler and maintains a safe default while allowing clients to avoid the copy if they are sufficiently smart and already null terminated.

rkayaith updated this revision to Diff 502249.Mar 3 2023, 1:36 PM
rkayaith edited the summary of this revision. (Show Details)

use flag instead of separate functions

rriddle accepted this revision.Mar 3 2023, 1:37 PM
lattner accepted this revision.Mar 3 2023, 1:39 PM

Nice, thank you!

rkayaith edited the summary of this revision. (Show Details)Mar 3 2023, 2:02 PM
This revision was landed with ongoing or failed builds.Mar 3 2023, 2:03 PM
This revision was automatically updated to reflect the committed changes.