Jordan, Yes. With that one comment added, the entire series looks good to me..
Reviewed-by: Michael Kinney <michael.d.kin...@intel.com> Mike -----Original Message----- From: Justen, Jordan L Sent: Sunday, June 07, 2015 11:13 PM To: Kinney, Michael D; edk2-devel@lists.sourceforge.net Cc: Liu, Yingke D; Laszlo Ersek Subject: RE: [PATCH v3 4/8] BaseTools/UniClassObject: Verify valid UCS-2 chars in UTF-16 .uni files On 2015-06-07 20:44:16, Kinney, Michael D wrote: > + > +TheUcs2Codec = Ucs2Codec() > > This is creating a global object in this module for the USC-2 codec. > Needs a comment to describe this. Ok. How about: ## Instance of Ucs2Codec class # # This object is used to support a codec for UCS-2 encoding # # The Ucs2Search function uses this object when a search for the usc-2 # codec is requested. # > + # > + # We currently only support UTF-16 > + # > + Encoding = 'utf-16' > > This comment is confusing because the patch allows UNI files to be > in different encodings. Is this really referring to the encoding > used internally to manage content read from an external file? In two patches later, "BaseTools/UniClassObject: Support UTF-8 string data in .uni files" (patch 6/8 for v3 and 06/10 for v4), I update this comment to: "Detect Byte Order Mark at beginning of file. Default to UTF-8" That is the patch where we start to accept more than just UTF-16 input files. With the comment above added, and this explaination, do you think the patch is good enough to add your Reviewed-by signature? How about the other patches in the series? Thanks for your time, -Jordan ------------------------------------------------------------------------------ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
